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

Fix and optimize arguments handling #72

Merged
merged 4 commits into from Aug 1, 2020
Merged

Conversation

marcandre
Copy link
Contributor

@marcandre marcandre commented Jul 18, 2020

When looking at some cops' performance I realized that #arguments? seemed to take non-negligible time. I realized that it creates an array, only to later check if it's empty. This PR optimizes this, for SendNode and others like it (modern mode only).

SuperNode, DefinedNode and YieldNode were worse, in that two temporary arrays were created for each argument call (including #arguments?) and are also (trivially) optimized.

This fixes LambdaNode (modern only) for arguments and #receiver

# Same functionality as ParameterizedNode, but optimized for Nodes
# where `arguments` are `children[first_argument_index..-1]`
#
module ParameterizedTrailNode
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite get the "trail" terminology.

Copy link
Contributor Author

@marcandre marcandre Jul 19, 2020

Choose a reason for hiding this comment

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

The first few children are not the arguments, only the trailing ones are.
I don't like ParameterizedNode, and I dislike ParameterizedTrailNode even more 😅, but I'm severly uninspired. Suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bbatsov is TrailingArgumentsNode a better name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @bbatsov

Copy link
Contributor

Choose a reason for hiding this comment

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

It gets a bit hairy, I guess, because the naming of the other nodes make reference to Ruby and its syntax, while this is a reference to the Parser output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Drenmi right, but there is still shared handling of the AST like in WrappedArgumentsNode (that isn't just an optimization)

One way would be to have ParameterizedNode::SingleChildArguments / ParameterizedNode::LastChildrenArguments and ParameterizedNode::WrappedArguments or similar, to denote the 3 different ways Parser encodes arguments?

This could even be handled automatically by ParameterizedNode when there's a def first_argument_index or similar that is encountered, but it feels too much.

@marcandre marcandre mentioned this pull request Jul 30, 2020
@marcandre marcandre changed the title SendNode: Optimize arguments handling Fix and optimize arguments handling Aug 1, 2020
@marcandre
Copy link
Contributor Author

marcandre commented Aug 1, 2020

I've refactored this a bit to improve the naming.

Relevant nodes can still include ParameterizedNode, or may include the variants ParameterizedNode::RestArguments or ParameterizedNode::WrappedArguments depending on how their arguments are handled. I added to the documentation which method each rely on (if any).

I'd like to merge this if it sounds ok and cut a release of rubocop-ast...

Copy link
Contributor

@bbatsov bbatsov left a comment

Choose a reason for hiding this comment

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

I like the current version.

@marcandre marcandre merged commit 11ef5c0 into rubocop:master Aug 1, 2020
@marcandre marcandre deleted the arguments branch August 1, 2020 20:21
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.

None yet

3 participants