fix(codegen): escape </script
#11782
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
xu-cheng
force-pushed
the
codgen_escape_script
branch
3 times, most recently
from
8d800f6 to
47c7866
Compare
|
Thanks very much for doing this! I'm tied up next few days, but will endeavor to review (and hopefully merge) early next week. |
CodSpeed Instrumentation Performance ReportMerging #11782 will improve performances by 13.32%Comparing Summary
Benchmarks breakdown
|
xu-cheng
force-pushed
the
codgen_escape_script
branch
from
47c7866 to
5a2a3a9
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
xu-cheng
force-pushed
the
codgen_escape_script
branch
3 times, most recently
from
7865eef to
3223051
Compare
xu-cheng
force-pushed
the
codgen_escape_script
branch
from
3223051 to
922f770
Compare
overlookmotel
force-pushed
the
codgen_escape_script
branch
from
922f770 to
1e860a2
Compare
overlookmotel
approved these changes
Jul 2, 2025
Comment on lines
+729
to
+737
| // Compiler condenses these operations to an 8-byte read, u64 AND, and u64 compare. | ||
| // https://godbolt.org/z/oGG16fK6v | ||
| let mut slice: [u8; 8] = slice.try_into().unwrap(); | ||
| for b in slice.iter_mut().skip(2) { | ||
| // `| 32` converts ASCII upper case letters to lower case. | ||
| *b |= 32; | ||
| } | ||
|
|
||
| slice == *b"</script" |
There was a problem hiding this comment.
Nice! You shaved a branch off my version.
This was referenced Jul 2, 2025
graphite-app bot
pushed a commit
that referenced
this pull request
Jul 2, 2025
Follow-on after #11782. Clarify comments.
graphite-app bot
pushed a commit
that referenced
this pull request
Jul 2, 2025
Follow-on after #11782. Add `#[inline(always)]` to `is_script_close_tag`, to ensure compiler can deduce the slice is 8 bytes long, and remove a branch.
graphite-app bot
pushed a commit
that referenced
this pull request
Jul 3, 2025
Follow-on after #11782. That PR fixed escaping `</script` in strings, but it regressed some codegen benchmarks by 2%. Optimize the string search to win some of that perf back by: 1. Doing a preliminary search for `<` first, and only the more expensive search for `</script` once a `<` is found. 2. Searching longer strings for `<` in chunks of 16 bytes, using SIMD.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #10334.
Replace #10340 to cover the escape for both template literals and comments. We don’t need to handle regex, which is already covered by existing codegen.
Please let me know anything needed to merge this PR. Thanks.