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

take the fast path for Regexp.new with non-default options #4731

Merged
merged 7 commits into from
Oct 15, 2021

Conversation

froydnj
Copy link
Contributor

@froydnj froydnj commented Oct 14, 2021

Motivation

I noticed that we weren't compiling some instance of constant regexps, like /pattern/x to a pre-compiled regexp, but instead we were evaluating everything at runtime with Regexp.new. You can imagine that this was quite a bit slower than the interpreter.

Desugar was initially to blame, because we were creating the options argument for Regexp.new with a chain of or'd-together constants: the compiler expected to see a single constant integer argument, or it wouldn't take the fast path. Makes sense, let's fix that.

That turned up another problem: Payload::cPtrToRubyString wasn't ready to cope with an options that wasn't 0, because the variable name it created to hold the precompiled regexp was only based on the pattern. So regexps with the same pattern but different options would have colliding variable names, with predictable consequences. Fixing that was as easy as adding the option (as an integer, not as the original letter names) to the variable name.

Finally, I added some filecheck tests to try and ensure that we generate the fast path for interesting combinations of regexp options. Ruby supports other options, but desugar says they all should have been handled by the parser, so we don't need to check them here. We also already have runtime checks for non-standard options to ensure they behave the same way as the interpreter, so I didn't need to add extra tests for them.

Test plan

See included automated tests.

@froydnj froydnj requested a review from a team as a code owner October 14, 2021 21:27
@froydnj froydnj requested review from elliottt and removed request for a team October 14, 2021 21:27
@froydnj
Copy link
Contributor Author

froydnj commented Oct 14, 2021

We have a policy of testing changes to Sorbet against Stripe's codebase before
merging them. I've kicked off a test run for the current PR. When the build
finishes, I'll share with you whether or how it failed. Thanks!

Stripe employees can see the build results here:

https://go/builds/bui_KPQLhwJukxxWHm
https://go/builds/bui_KPQLNVmk7kODQl

Copy link
Contributor

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

This is a great find! What do you think about calling the methods you defined in the test so that we can verify that the flags are being interpreted correctly?

@froydnj
Copy link
Contributor Author

froydnj commented Oct 15, 2021

This is a great find! What do you think about calling the methods you defined in the test so that we can verify that the flags are being interpreted correctly?

I added some more tests to Regexp.rb.

@froydnj
Copy link
Contributor Author

froydnj commented Oct 15, 2021

This is a great find! What do you think about calling the methods you defined in the test so that we can verify that the flags are being interpreted correctly?

I added some more tests to Regexp.rb.

Thinking about this more, it's even better to add tests to regexp_flags.rb (and probably what you meant in the first place), so I did that too.

@froydnj froydnj enabled auto-merge (squash) October 15, 2021 13:24
@froydnj froydnj merged commit ab2ddc2 into master Oct 15, 2021
@froydnj froydnj deleted the froydnj-regexp-fast-path branch October 15, 2021 13:53
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