Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Use FastArray #8133

wants to merge 1 commit into from

Conversation

marcandre
Copy link
Contributor

Same as rubocop/rubocop-ast#29 for main gem.

@marcandre marcandre force-pushed the tuple branch 3 times, most recently from 68f8133 to 6535b6f Compare June 10, 2020 17:09
@marcandre
Copy link
Contributor Author

@rubocop-hq/rubocop-core What do you think of this?

I measured a ~5% gain on rubocop's code (with this and same for rubocop-ast), which is more than I was hoping for.

@marcandre
Copy link
Contributor Author

Note: I'll remove the double implementation and just bump the required rubocop-ast once it's released.

@koic
Copy link
Member

koic commented Jun 11, 2020

Speeding up is attractive :-) On the other hand, Array#freeze is a common method. I'm not sure it's a good approach to replace the common method with the original API Tuple for speeding up.

@marcandre
Copy link
Contributor Author

@koic Sorry, I don't understand. These Tuple are always frozen (see initialize). I'll amend the PR to better document the class.

@koic
Copy link
Member

koic commented Jun 11, 2020

My explanation is not that good 💦

Array#freeze is a familiar way of making frozen object.

    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]

Array#freeze may be slower than Tuple, but Array#freeze is clear. I think Array#freeze is a preferred readable code.

I'd like to hear opinions of other maintainers.

@bquorning
Copy link
Contributor

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.

@marcandre
Copy link
Contributor Author

marcandre commented Jun 11, 2020

I think I need an explanation of why regular arrays are so comparatively slow

It's Array#include? that is slow, as it is O(n) for n entries, versus O(1) for Set#include?. If we take Node#conditional? for example, it checks if the type is equal to :lvasgn? No, ok, is it equal to :ivasgn then? No, ok, how about :cvasgn? Neither, ok, so maybe :gvasgn? Etc. (there are 5 more...). When I think that each call goes through the whole list, I cry a little bit inside 😿

Set#include? uses the magic of Hash map that can determine inclusion in the same time for a long or short list of symbols.

I can understand that using a custom class instead of an Array introduces a little extra notion (Tuple == fast-frozen-array), but I don't think it's a big cognitive load, and it's not really mandatory to know since those constants are not really meant to be used directly. Note that python has tuples (i.e. immutable arrays), so anyone knowing python will know intuitively that it's an immutable array.

@marcandre
Copy link
Contributor Author

@bquorning Our strings are already frozen: %w[Any Thing Here].first.frozen? # => true

@marcandre marcandre force-pushed the tuple branch 2 times, most recently from bf6dbe0 to 5628c24 Compare June 24, 2020 01:41
@marcandre marcandre changed the title Use Tuple Use FastArray Jun 24, 2020
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.

4 participants