Skip to content

Introduce RuboCop Performance #316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 13, 2025

Conversation

tagliala
Copy link
Contributor

@tagliala tagliala commented Aug 1, 2025

What this does

Hello, thanks for your work on this library

This is my first contribution here. I think that adding RuboCop Performance plugin this can benefit on the long term

Each offense has been fixed in an individual commit, with explanation and benchmarks, where possible.

Some cops have been temporarily disabled because they can break API

Type of change

  • Bug fix
  • New feature
  • Breaking change ⚠️ ⚠️ ⚠️ not at the best of my knowledge
  • Documentation
  • Performance improvement

Scope check

  • I read the Contributing Guide
  • This aligns with RubyLLM's focus on LLM communication
  • This isn't application-specific logic that belongs in user code
  • This benefits most users, not just my specific use case

Quality check

  • I ran overcommit --install and all hooks pass
  • I tested my changes thoroughly
  • I updated documentation if needed
  • I didn't modify auto-generated files manually (models.json, aliases.json)

API changes

  • Breaking change
  • New public methods/classes
  • Changed method signatures
  • No API changes

Related issues

@tagliala tagliala force-pushed the feature/introduce-rubocop-performance branch 4 times, most recently from fb0c645 to f9c9af8 Compare August 7, 2025 14:38
Disable `CollectionLiteralInLoop` cop in specs

Automatically fix safe offenses:
- Performance/BindCall
- Performance/DoubleStartEndWith
- Performance/StringReplacement

```
rubocop --only Performance/BindCall,Performance/DoubleStartEndWith,Performance/StringReplacement -a
```
`String#include?` is considerably more efficient than `String#match?`.

However, for uniformity and future-proofing with minimal modifications,
it is preferable to permit that comparison in capabilities detection.
Implementing a comprehensive fix for this issue may be regarded as a
disruptive change due to its impact on the API.

Specifically, a partial solution will introduce
`Lint/UnusedMethodArgument` violations, which can be disregarded.

However, benchmarking indicates that removing the `&block` argument
from the method signature will result a performance improvement:

```
ruby 3.4.5 (2025-07-16 revision 20cda200d3) +PRISM [arm64-darwin24]

Comparison (IPS):
     yield (no args): 10747202.2 i/s
  yield (&block arg):  9308969.9 i/s - 1.15x  (± 0.00) slower
          block.call:  8607805.1 i/s - 1.25x  (± 0.00) slower
```

Ref:
- https://github.com/fastruby/fast-ruby?tab=readme-ov-file#proccall-and-block-arguments-vs-yield-code
```
rubocop --only Performance/MapCompact -A
```

These changes are safe because both `providers` and `capabilities` are a `Hash`,
so they both include `Enumerable` and respond to `filter_map`

Ref:
- https://ruby-doc.org/core-3.1.0/Enumerable.html#method-i-filter_map
While `String#dup` is as fast as `+''` in Ruby >= 3.3, this is not the
case for `String.new`, so this cop has been applied only in production
code.

RuboCop considers this change unsafe because `+''` returns an UTF-8
string, while `String.new` returns an ASCII string.

In this context, this should not be a breaking change.

```
rubocop --only Performance/UnfreezeString -A
```

A manual fix has been applied in ActsAs concern to use empty string
literal for ActiveRecord `content` assignment instead of `String.new` or
`+''`. Passing `''` directly to ActiveRecord is sufficient since the
string isn't mutated.

Bench:

```
ruby 3.4.5 (2025-07-16 revision 20cda200d3) +PRISM [arm64-darwin24]

Comparison (IPS):
                 +'': 15938643.5 i/s
          String.new: 11016752.0 i/s - 1.45x  (± 0.00) slower
```

Ref:
- https://docs.rubocop.org/rubocop-performance/cops_performance.html#performanceunfreezestring
- https://github.com/fastruby/fast-ruby?tab=readme-ov-file#stringdup-vs-string-code
There is no auto-correction for this offense, so it has been fixed
manually

Ref: https://github.com/fastruby/fast-ruby#normal-way-to-apply-method-vs-method-code
@tagliala tagliala force-pushed the feature/introduce-rubocop-performance branch from f9c9af8 to 4a33aa5 Compare August 13, 2025 13:49
Copy link

codecov bot commented Aug 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.75%. Comparing base (b2855b4) to head (4a33aa5).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #316   +/-   ##
=======================================
  Coverage   87.75%   87.75%           
=======================================
  Files          88       88           
  Lines        3480     3480           
  Branches      724      724           
=======================================
  Hits         3054     3054           
  Misses        426      426           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Owner

@crmne crmne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@crmne crmne merged commit ccd7915 into crmne:main Aug 13, 2025
15 checks passed
@tagliala tagliala deleted the feature/introduce-rubocop-performance branch August 14, 2025 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants