-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Use FastArray #8133
Use FastArray #8133
Conversation
68f8133
to
6535b6f
Compare
@rubocop-hq/rubocop-core What do you think of this? I measured a ~5% gain on rubocop's code (with this and same for |
Note: I'll remove the double implementation and just bump the required |
Speeding up is attractive :-) On the other hand, |
@koic Sorry, I don't understand. These |
My explanation is not that good 💦
COMMON_PARAMS = %w[Exclude Include Severity inherit_mode
AutoCorrect StyleGuide Details].freeze On the other hand, I think Tuple() is a non-standard way of making frozen object. COMMON_PARAMS = Tuple %w[Exclude Include Severity inherit_mode
AutoCorrect StyleGuide Details]
I'd like to hear opinions of other maintainers. |
You would need to freeze all array elements too, to make it equivalent. COMMON_PARAMS = %w[Exclude Include Severity inherit_mode
AutoCorrect StyleGuide Details].map(&:freeze).tap(&:freeze) But I tend to agree with @koic. It’s a tradeoff between speed, readability and understandability. I think I need an explanation of why regular arrays are so comparatively slow. And which of the changed arrays have the most impact on the change to performance. If it’s just a single hotspot, perhaps we can find another way of optimising it. |
It's
I can understand that using a custom class instead of an |
@bquorning Our strings are already frozen: |
bf6dbe0
to
5628c24
Compare
Same as rubocop/rubocop-ast#29 for main gem.