perf(lexer): dedupe numeric separator check

[DonIsaac]

What This PR Does

Updates numeric literal token lexing to record when separator characters (_) are found in a new Token flag. This then gets passed to parse_int and parse_float, removing the need for a second _ check in those two functions.

When run locally, I see no change to lexer benchmarks and minor improvements to codegen benchmarks. For some reason, semantic and source map benches seem to be doing slightly worse.

Note that I attempted to implement this with bitflags! (making escaped and is_on_newline flags as well) and this caused performance degradation. My best guess is that it turned reads on these flags from a mov to a mov + a binary and.

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

[codspeed-hq]

CodSpeed Performance Report

Merging #3283 will improve performances by 5.83%

_{Comparing don/perf/lexer-dedupe-sep-check (7fd434e) with main (dad47a5)}

Summary

⚡ 6 improvements
✅ 21 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	don/perf/lexer-dedupe-sep-check	Change
⚡	lexer[RadixUIAdoptionSection.jsx]	90.2 µs	86 µs	+4.87%
⚡	lexer[antd.js]	113.5 ms	107.8 ms	+5.31%
⚡	lexer[cal.com.tsx]	28.2 ms	26.7 ms	+5.41%
⚡	lexer[checker.ts]	66.6 ms	63.6 ms	+4.75%
⚡	lexer[pdf.mjs]	18.6 ms	17.6 ms	+5.83%
⚡	semantic[pdf.mjs]	141.6 ms	136.5 ms	+3.7%

[Boshen]

We can always move the underscore string out if the token gets filled up with other data.

oxc/crates/oxc_parser/src/lexer/mod.rs

Lines 91 to 96 in dad47a5

	/// Data store for escaped strings, indexed by [Token::start] when [Token::escaped] is true
	pub escaped_strings: FxHashMap<u32, &'a str>,
	/// Data store for escaped templates, indexed by [Token::start] when [Token::escaped] is true
	/// `None` is saved when the string contains an invalid escape sequence.
	pub escaped_templates: FxHashMap<u32, Option<&'a str>>,

[overlookmotel]

@DonIsaac Very nice work!

I didn't get a chance to review this before it was merged (everything happened in the middle of the night in my timezone, and I was asleep). Can I make a request for a follow-on?

The one thing I'd like to pick up on is the use of unsafe. I understand why you've made parse_float_without_underscores_unchecked an unsafe function to draw attention to the precondition. But personally I think we should reserve unsafe for cases where the preconditions MUST be met or it's UB.

In my mind, there's quite a big difference between:

(a) this function may provide nonsense output or panic if you give it the wrong input, and
(b) this function may do basically anything if you give it the wrong input, and it may cause errors or malfunctions in unrelated parts of the codebase (UB).

In the potential-UB cases, the person using the API should take it slow, rigorously check all the preconditions, and document how they can prove the preconditions are satisfied. In my view we want to keep this to an absolute minimum otherwise it's like "crying wolf" - people will get used to calling unsafe functions, and will get blasé and just write // SAFETY: All conditions are satisfied, even for the cases where they really should be being careful.

I would also like to institute a policy where any PR using unsafe can only be merged after a more rigorous than usual code review, by multiple reviewers. Again, we'd need to keep this to a minimum, or it'd be impractical.

So... I've probably explained this in more detail than I needed to (sorry, I have a tendency to talk too much!). But the short version is: If you can understand my logic and agree, would you mind doing a follow-on PR to remove the usage of unsafe here?

[overlookmotel]

Second thing:

I don't understand the debug_assert! in set_has_separator. Why the addition of || self.kind == Kind::default()?

[overlookmotel]

Some side notes:

Lexer benchmarks don't really matter. The lexer is not exposed to user as a stand-alone API, so it's the parser benchmark which actually matter. The lexer benchmark is only there for our internal use to give greater visibility when working on the lexer. The flame graphs on CodSpeed for lexer are much easier to interpret than the parser's.

In the case of this PR, the real gain is the +1% on the parser. In one sense 1% isn't much, but I think it's significant. Multiple incremental gains of 1% add up fast.

I am mystified as to why we're seeing 5% gain on lexer benchmarks here. If anything, you'd expect the lexer to be slightly slower because it's doing a little bit more work, in order to save work later for the parser.

The only thing I can think of is that changing one of the padding bytes in Token from u8 to bool somehow improves codegen. Would you like to replace the other 2 padding bytes with bools too, and see what happens?

Or do you have any idea what's going on here?

[DonIsaac]

@overlookmotel

1. Yes, I will make a separate PR removing unsafe
2. That check is needed since self.token is currently under construction and has a kind of Kind::Eof. It won't have a numeric kind until the token kind is set by the same methods consuming set_has_separator
3. I'm really not sure; these improvements baffled me as well. I think your hypothesis is solid; I can't think of anything else that may affect it. I'll try making _padding2 a [bool; 4] and see what effects that has.

[overlookmotel]

1. Yes, I will make a separate PR removing unsafe

Thank you!

2. That check is needed since self.token is currently under construction and has a kind of Kind::Eof. It won't have a numeric kind until the token kind is set by the same methods consuming set_has_separator

Ah ha that makes sense now.

3. I'm really not sure; these improvements baffled me as well. I think your hypothesis is solid; I can't think of anything else that may affect it. I'll try making _padding2 a [bool; 4] and see what effects that has.

I don't know if my hypothesis is solid or not! It's very mysterious.

This is all real micro-opt stuff. It only makes a difference because creating tokens is the majority of what the lexer does, so it's the definition of a hot path.

[overlookmotel]

I think I may have solved the mystery! Please see #3289 (comment)