Skip to content

Only validate the RegExp, do not optimize/compile it#4451

Merged
hansl merged 1 commit intoboa-dev:mainfrom
hansl:fix-4432
Oct 3, 2025
Merged

Only validate the RegExp, do not optimize/compile it#4451
hansl merged 1 commit intoboa-dev:mainfrom
hansl:fix-4432

Conversation

@hansl
Copy link
Contributor

@hansl hansl commented Oct 1, 2025

The regress package does export a try_parse method that tries to parse without optimizing and compiling the regex itself.

Notes: #4432 does specify that only parsing could improve performance.

@hansl hansl requested a review from a team October 1, 2025 22:46
@codecov
Copy link

codecov bot commented Oct 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.51%. Comparing base (6ddc2b4) to head (5dfc773).
⚠️ Report is 544 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4451      +/-   ##
==========================================
+ Coverage   47.24%   51.51%   +4.27%     
==========================================
  Files         476      504      +28     
  Lines       46892    51369    +4477     
==========================================
+ Hits        22154    26464    +4310     
- Misses      24738    24905     +167     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jedel1043
Copy link
Member

jedel1043 commented Oct 2, 2025

Huh, I wonder if we could store that as our AST node, then run the rest of the compilation pipeline at bytecode compilation time (maybe even lazily optimizing/compiling at runtime?)

@hansl
Copy link
Contributor Author

hansl commented Oct 2, 2025

@jedel1043 We cannot do that specifically. Only internal methods of Regex can use the intermediate representation. We could either make a PR to regress to expose those, or keep the Regex object itself in the AST.

I tried keeping the Regex object around but it ended up being quite complicated. I have a slightly better understanding of the AST now, so I could try. But this PR could still go in (it doesn't close the issue) as an intermediate step for performance.

Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

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

LGTM, I agree about the complexity of keeping the result, I looked into it aswell months ago and requires a large refactor in places to pass the value through. We can always merge this now and attempt that at a later date

@hansl hansl added this pull request to the merge queue Oct 3, 2025
Merged via the queue into boa-dev:main with commit 0468498 Oct 3, 2025
16 checks passed
@hansl hansl deleted the fix-4432 branch October 3, 2025 01:58
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.

3 participants