Demangle using fixed-length disambiguators - #94
Conversation
| let hex_len = self.0.bit_width().div_ceil(4).max(1); | ||
| let fill_len = 16 - (hex_len as usize); | ||
| out.write_str(&Self::MAX_HEX_FILL[0..fill_len])?; | ||
| fmt::LowerHex::fmt(&self.0, out) |
There was a problem hiding this comment.
write!(out, "{:016x}", self.0) would work too, right?
There was a problem hiding this comment.
Unfortunately this is core-only code:
Lines 5 to 12 in f36e298
There was a problem hiding this comment.
write!() is in core. I don't know if the reason given for avoiding write!() in #50 is still fully valid given that we had a change to the fmt implementation since:
The second commit is mostly stylistic, trying to enforce a pattern of "not using write! in demangling logic", and only using fmt::Trait::fmt(&value, self.out) - which I'm guessing we do want (for performance reasons?).
In any case I guess the current code is fine.
There was a problem hiding this comment.
I have a slight preference for the standard fixed-width code, it's more maintainable and less likely to have bugs.
But my custom code benchmarks at:
test v0::tests::bench_demangle_crate_with_zero_disambiguator ... bench: 173.72 ns/iter (+/- 8.86)
test v0::tests::bench_short_crate_disambiguator_bug ... bench: 3,842.77 ns/iter (+/- 69.09)
And format_args! (the expansion of write!) is slightly slower:
test v0::tests::bench_demangle_crate_with_zero_disambiguator ... bench: 175.11 ns/iter (+/- 6.15)
test v0::tests::bench_short_crate_disambiguator_bug ... bench: 4,005.31 ns/iter (+/- 240.34)
Specifically, it expands to out.write_fmt(core::format_args!("{:016x}", self.0)).
I'm using a MacOS M1 laptop, so I don't know if these results carry across. But it seems like custom code may still be a winner over write!. I'll add a comment to explain this, and push an update.
46011df to
73a7298
Compare
|
My benchmarking code is here: No other tests format the crate hash, which explains why we never picked up this bug. |
Most UI tests expect a 16-character disambiguator, but rustc-demangle omits leading zeroes.
This can cause test failures on ~1/16 of the changes to demangling tests.
For example: rust-lang/rust#160050 (comment)
This PR adds a
Disambiguatorwrapper type, so it is impossible to format it incorrectly.I used https://www.dcode.fr/base62-encoding to generate the encoded examples, you need to select the second alphabet (lowercase first).