Skip to content

perf(parser): avoid checkpoint when parsing left curly in jsx#11377

Merged
Boshen merged 2 commits intooxc-project:mainfrom
ulrichstark:avoid-checkpoint-when-parsing-curly-in-jsx
May 30, 2025
Merged

perf(parser): avoid checkpoint when parsing left curly in jsx#11377
Boshen merged 2 commits intooxc-project:mainfrom
ulrichstark:avoid-checkpoint-when-parsing-curly-in-jsx

Conversation

@ulrichstark
Copy link
Contributor

This requires parse_jsx_expression_container and parse_jsx_spread_child not to expect LCurly because that's already bumped by the caller. Is that okay? Do I need to update the doc comments of both functions? Let's see if there are improvements in the benchmark...

Part of #11334

@github-actions github-actions bot added A-parser Area - Parser C-performance Category - Solution not expected to change functional behavior, only performance labels May 29, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented May 29, 2025

CodSpeed Instrumentation Performance Report

Merging #11377 will not alter performance

Comparing ulrichstark:avoid-checkpoint-when-parsing-curly-in-jsx (b1a237b) with main (1cd8b9c)

Summary

✅ 38 untouched benchmarks

@Sysix Sysix changed the title perf(linter): avoid checkpoint when parsing left curly in jsx perf(parser): avoid checkpoint when parsing left curly in jsx May 29, 2025
@leaysgur
Copy link
Member

FYI: I also tried to eliminate the rewind() for {expr and {...expr in #11314 (a12ff21), but there was no visible improvement. 😓

@Boshen Boshen self-assigned this May 30, 2025
@Boshen
Copy link
Member

Boshen commented May 30, 2025

Codspeed may not hit these code paths, let me double check.

Copy link
Member

@Boshen Boshen 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 really good change. I added a small change to avoid to at + bump check.

@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label May 30, 2025
@graphite-app
Copy link
Contributor

graphite-app bot commented May 30, 2025

Merge activity

  • May 30, 6:57 AM UTC: @ulrichstark we removed the merge queue label because we could not find a Graphite account associated with your GitHub profile.

You must have a Graphite account in order to use the merge queue. Create an account and try again using this link

@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label May 30, 2025
@ulrichstark
Copy link
Contributor Author

This is a really good change. I added a small change to avoid to at + bump check.

Thanks @Boshen! So I assume the doc comments of both functions are still fine and need no update.

Does it help when I create a Graphite account or can I just ignore those messages that I was removed from the merge queue?
Or is this only for core members?
Last time I tried creating a Graphite account and linking it to the oxc repo didn't work.

@Boshen Boshen merged commit 24aba18 into oxc-project:main May 30, 2025
24 checks passed
@ulrichstark ulrichstark deleted the avoid-checkpoint-when-parsing-curly-in-jsx branch May 30, 2025 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-parser Area - Parser C-performance Category - Solution not expected to change functional behavior, only performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants