chore: use const fn in Span #10320
chore: use const fn in Span #10320
Conversation
|
CodSpeed Performance ReportMerging #10320 will degrade performances by 2.42%Comparing Summary
Benchmarks breakdown
|
| self.lo | ||
| } | ||
|
|
||
| #[inline] | ||
| pub fn new(mut lo: BytePos, mut hi: BytePos) -> Self { | ||
| if lo > hi { |
There was a problem hiding this comment.
Actually #9962 is about removing this swap
There was a problem hiding this comment.
I see. But I have two questions:
• Does this change break existing code?
• Should we add assert!(lo < hi) or debug_assert!(lo < hi)?
There was a problem hiding this comment.
Yes for the first question. (It’s a bug IMO)
And debug assert would be enough considering the number of parser tests
There was a problem hiding this comment.
Moving swap to the caller code will make it slower. That's why simply changing it to a const fn slowed it. (swap operation is inlined into call side with constant swap)
crates/swc_common/src/syntax_pos.rs
Outdated
| if lo > hi { | ||
| std::mem::swap(&mut lo, &mut hi); | ||
| pub const fn new(lo: BytePos, hi: BytePos) -> Self { | ||
| if lo.0 > hi.0 { |
There was a problem hiding this comment.
This should be removed and callers should be fmodified to call Span::new with correct argument
|
The latest benchmark report doesn’t show any regressions (I’m guessing it’s just some noise in the Not sure why the @kdy1 What do you think? Should we keep pursuing this change? |
Description:
Using
const fncan generally help lower both runtime overhead.BREAKING CHANGE:
nothing.
Related issue (if exists):
Fixes #9962