Remove unnecessary string cloning from the parser - #9884
Conversation
|
c9054e1 to
efc7d6e
Compare
|
Hate this change, didn't even improve benchmarks!!! |
efc7d6e to
bc1e449
Compare
That's because codspeed uses the fixed cost 0 for allocations.... |
| } | ||
| content.push(ch); | ||
| fn parse_bytes(mut self) -> Result<StringType, LexicalError> { | ||
| let index = first_non_ascii_byte(self.source.as_bytes()); |
There was a problem hiding this comment.
@BurntSushi - Is this exposed in any crates that we already use? I had to vendor it from bstr.
There was a problem hiding this comment.
| // Add the characters before the escape sequence to the string. | ||
| let before_with_slash = self.skip_bytes(index + 1); |
There was a problem hiding this comment.
nit: the index could also include { or } as well, right? The variable name before_with_slash suggests otherwise unless I'm wrong.
bc1e449 to
2e75d58
Compare
CodSpeed Performance ReportMerging #9884 will improve performances by 6.53%Comparing Summary
Benchmarks breakdown
|
2e75d58 to
f065e8d
Compare
|
I'm gonna get to the bottom of this if it's the last thing I do. |
f065e8d to
f0cdc8f
Compare
|
I see basically no improvement on local benchmarks either via criterion or hyperfine. I'm not sure why, to be honest. I guess I'll proceed with the change since it's both (1) a better API, and (2) intuitively better to avoid re-allocating here when we don't need to. But I'm a bit confused. |
|
(Ignore current state of the PR, I duplicated the existing and updated code to do some benchmarking. I'll clean up later.) |
|
By removing lexing from the parser benchmark, I'm now able to see a more consistent 1-2% improvement in parse time, so I'll move forward with this. (Benchmarking the individual string-parsing functions (rather than the parser as a whole) shows an enormous speed-up, like 2x.) |
a34458d to
82e1d4e
Compare
82e1d4e to
58178b3
Compare
4 participants
Closes #9869.