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

Implement PCRE2 JIT compilation #12866

Merged
merged 10 commits into from
Jan 5, 2023
Merged

Implement PCRE2 JIT compilation #12866

merged 10 commits into from
Jan 5, 2023

Conversation

straight-shoota
Copy link
Member

Adds JIT support to the PCRE2 engine introduced in #12856.

Base automatically changed from feature/pcre2 to master December 22, 2022 20:35
This was referenced Dec 22, 2022
# can't be any concurrent access to the JIT stack.
@[ThreadLocal]
class_getter jit_stack : LibPCRE2::JITStack do
jit_stack = LibPCRE2.jit_stack_create(32_768, 1_048_576, Regex::PCRE2.general_context)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe for a follow up, but these magic numbers could be or'ed with the ENV in order to have some control. I'm thinking in particular of memory-restricted apps.

when .jit_badoption?
# okay
else
raise ArgumentError.new("Regex JIT compile error: #{error}")
Copy link
Member

@beta-ziliani beta-ziliani Jan 3, 2023

Choose a reason for hiding this comment

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

Isn't this a bit too harsh? Without compilation the application might still work, even if slow. This said, I'm not sure how could we let the user know that something went wrong.

Copy link
Member Author

@straight-shoota straight-shoota Jan 3, 2023

Choose a reason for hiding this comment

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

I'm not sure what errors can actually reasonably appear here. When JIT compilation is not supported, that's indicated by ERROR_JIT_BADOPTION and handled in the previous branch (we ignore that).
Anything else, I don't know. But I would probably expect that could be more fatal errors where you may not be able to just continue on.
A more plausible point for ignoring errors might be when JIT stack allocation fails (jit_stack_create).

Copy link
Member

Choose a reason for hiding this comment

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

PCRE2_ERROR_NOMEMORY is another one we might want to not raise. I wonder if it makes sense to just ignore any error and have a @compiled : Bool to know if it was compiled or not

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

We can work out details for the two remaining itches in follow-ups. Let get this rolling first.

@straight-shoota straight-shoota added this to the 1.7.0 milestone Jan 4, 2023
@straight-shoota straight-shoota merged commit c9f4a64 into master Jan 5, 2023
@straight-shoota straight-shoota deleted the feature/pcre2-jit branch January 5, 2023 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants