Configure Style/NumericPredicate for performance

[tagliala]

Hello,

While the Style/NumericPredicate cop recommends using .zero?, this can lead to performance degradation in gems where zero checks are frequent and speed-critical; for such performance-oriented libraries, using == 0 remains preferable despite not adhering to the style guide.

This will not lead to performance improvements for the current implementation of memo_wise, I'm rather submitting this as a "reminder"

---

Replace predicate methods like v.zero? with direct comparison v == 0 for better performance.

While the current implementation may not show visible improvements, frequently called methods using .zero? instead of == 0 can lead to significant performance losses.

Benchmarks across Ruby versions 2.5 to 3.3 consistently show comparisons outperforming predicates:

Comparison (Ruby 2.6 to 3.2, v = 0):
              v == 0: 21479329.3 i/s
             v.zero?: 17979885.4 i/s - 1.19x  (± 0.00) slower

Comparison (Ruby 2.5 and 3.3, v = 0):
              v == 0: 23652215.7 i/s
             v.zero?: 21843174.0 i/s - 1.08x  (± 0.00) slower

Comparison (Ruby 2.5 to 3.3, v = 1):
              v == 0: 23227474.2 i/s
             v.zero?: 21675200.9 i/s - 1.07x  (± 0.00) slower

Before merging:

• Copy the table printed at the end of the latest benchmark results into the README.md and update this PR
• If this change merits an update to CHANGELOG.md, add an entry following Keep a Changelog guidelines with semantic versioning

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ef52a1a) to head (50a5b0a).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #356   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          190       190           
  Branches        90        90           
=========================================
  Hits           190       190           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ms-ati]

Hi @tagliala! First I want to thank you for what appears to be an awareness campaign to help the community to understand where it may be better to adjust our Rubocop configs to trade-off for performance in hot loops, as opposed to an interest in maximum readability. Thank you!

Can you explain why we would make this change here, in these two lines of code? One is in benchmark set up, the other is in one-time instrumentation of an object with MemoWise.

In neither case, is it yet clear to me that our trade-off would be for performance at the cost of (arguable) readability. Is that trade-off clear to you here? If so, can you help explain and make it clear?

[tagliala]

Hi,

the other is in one-time instrumentation of an object with MemoWise.

In the specific case of memo wise, it is not going to provide any visible improvement, because as you mentioned it is a one time initialization. This is also stated in the opening post: "This will not lead to performance improvements for the current implementation of memo_wise, I'm rather submitting this as a "reminder"

Is that trade-off clear to you here? If so, can you help explain and make it clear?

It is more than clear. The fact is that this cop is globally configured for readability, so it may lead to an usage of numeric predicates in contexts where they are not optimal, like loops. Again, this is not the case. I see memo_wise as a gem oriented to performance, so I proposed this change here in specific, but as a reminder and also to read other thoughts

[JacobEvelyn]

Thanks for making this PR, @tagliala! I appreciate the thoughtful commit message and interest in making future gem changes more performant.

Personally, I'm indifferent to this change. I generally like having performant defaults, but I also struggle to see a world in which we would change this gem to include .zero? or .one? in a hot path at all, let alone have it slip through code review and our benchmarks. I'm not opposed to merging this however, since you took the time to make it.

@ms-ati do you have a preference?

[tagliala]

Welcome

I'm not opposed to merging this however, since you took the time to make it.

Thanks, and don't worry about the time.

I was also proposing to understand if that method could have made the difference in some circumstances, like large codebases, or if the new default may positively have impact on future development, but as far as I understand this is not a concern for memo_wise. And if it will ever be, it will be possible to make the change in the future

[ms-ati]

I was also proposing to understand if that method could have made the difference in some circumstances, like large codebases, or if the new default may positively have impact on future development, but as far as I understand this is not a concern for memo_wise. And if it will ever be, it will be possible to make the change in the future

Thanks @tagliala. @JacobEvelyn my (very slight) preference would be to not merge this, sharing reasoning with what you wrote above.

As you wrote above Jacob, we share a goal to encourage contributions. It sounds above as though @tagliala you are understanding that both of the maintainers in this thread absolutely do value the effort and communications you've made here. And that even if we do not merge this PR at this moment, we absolutely do encourage thoughtful and helpful contributions like this in the future!

If we are all comfortable with the decision, I'd be in favor of closing this PR.

[JacobEvelyn]

I was also proposing to understand if that method could have made the difference in some circumstances, like large codebases

On this point specifically I think it won't make a noticeable difference. That .zero? will typically run at load time (when memo_wise is called), and based on your benchmark it seems like both .zero? and == 0 can execute many billions of times per second. I can't see a codebase ever having close to that many memo_wise calls so I don't think this will matter.

Let me know if I'm missing something though!