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

Move blocks outside Lexer#advance #902

Merged

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Dec 21, 2022

@eregon
Copy link
Contributor Author

eregon commented Dec 21, 2022

It would be good to add a test to ensure Lexer#advance remains without block/proc/lambda/lambda-literal inside it, as it matters quite a bit for performance and compilation size/time.
I'm not able to do that now though (it's already past midnight here and I'm traveling tomorrow).

lib/parser/lexer.rl Outdated Show resolved Hide resolved
@eregon
Copy link
Contributor Author

eregon commented Dec 21, 2022

The failing test shows these lambda also access some ivars, so they can't be constants, I'll move them in initialize then.

* This gives big compilation size and time benefit, because the local
  variables of Lexer#advance are no longer captured by those blocks
  (even if unused due to Ruby semantics and `Proc#binding`).
* See whitequark#871 (comment)
@eregon eregon force-pushed the move-lambdas-outside-advance-method branch from 8e24cbc to 7c8a9ee Compare December 21, 2022 23:46
@eregon
Copy link
Contributor Author

eregon commented Dec 21, 2022

Done, CI should be green now.

@iliabylich iliabylich merged commit d5d648a into whitequark:master Dec 21, 2022
@iliabylich
Copy link
Collaborator

Thanks!

@eregon
Copy link
Contributor Author

eregon commented Dec 22, 2022

I missed that 5b2135e#diff-894f90ef5e566bf82a7b031fd82df1aece88eaac96cae6483feba8c020a63761R884 also did this partially in a slightly different way, which also works but has the slight perf disadvantage of allocating a Proc per such method call vs one per Lexer instance.
So, my apologies about the potential conflicts (should be just a few lines though).

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

2 participants