Implement Debug for C-like enums with a concatenated string#155452
Implement Debug for C-like enums with a concatenated string#155452makai410 wants to merge 1 commit into
Debug for C-like enums with a concatenated string#155452Conversation
|
Changes to the code generated for builtin derived traits. cc @nnethercote |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Implement `Debug` for C-like enums with a concatenated string
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (5a131e3): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -3.6%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.8%, secondary 2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.4%, secondary 0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 492.421s -> 491.503s (-0.19%) |
|
There was a leftover condition from an earlier PR that I pulled in without a proper review, such that it actually didn't apply the optimization to large enums <_<; I also want to try skipping bounds checks. I suppose it could improve runtime performance, at least on my machine, based on some pretty rough benchmarks with an enum of 10,000 variants. |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Implement `Debug` for C-like enums with a concatenated string
|
I'm happy to review this once it's ready. @rustbot author |
| let variant_names = def | ||
| .variants | ||
| .iter() | ||
| .map(|v| v.disr_expr.is_none().then_some(v.ident.name.as_str())) | ||
| .collect::<Option<ThinVec<_>>>()?; |
There was a problem hiding this comment.
Hmmm, I think I missed a valid case, considering:
enum Uwu {
QwQ = 0,
AwA = 1,
}which has explicit discriminants but is actually dense.
There was a problem hiding this comment.
Also, we can use an offset when some variants have negative discriminants while the overall variants remain dense.
I'll do a follow-up PR to implement these.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (7348f26): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.7%, secondary 0.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 4.4%, secondary 3.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.4%, secondary 1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 493.313s -> 504.472s (2.26%) |
|
This will probably run into #148423. Furthermore, I believe this is likely to cause unsoundness in actual programs, due to the |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Implement `Debug` for C-like enums with a concatenated string
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (5b7107a): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.2%, secondary 0.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeResults (primary -0.1%, secondary 1.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 525.47s -> 516.414s (-1.72%) |
I added bounds check back in the new commits, so in the worst scenario like #148423, it could panic or output an incorrect result but won't trigger UB, at least. So it depends on whether that's acceptable. |
|
I think the perf result looks ideal, not affecting most common cases and being able to improve cases when it actually matters. One more optimization we can do here is to use |
| fmt: Box<ast::Expr>, | ||
| ) -> Option<(ThinVec<ast::Stmt>, Box<ast::Expr>)> { | ||
| let variant_count = def.variants.len(); | ||
| if variant_count < 39 { |
There was a problem hiding this comment.
Give this constant a name, and add a comment explaining how it was chosen.
There was a problem hiding this comment.
Sorry for the late response. I'm trying to write a bench tool to decide this constant more seriously. For now this is just picked by my intuition, which is IMO not a very cool thing to say here :)
There was a problem hiding this comment.
FWIW I think it's totally fine to just write down that it's picked by intuition and measured. The point is to give the next person who's gonna look into this enough information to decide how to follow up on this, it doesn't have to be cool.
There was a problem hiding this comment.
lol I think it's too late :3 already got something going here: https://codeberg.org/makai410/debug-fmt-perf
just wait for me to write up a readme and get some data. looks like the threshold is probably around 10 based on my testing so far, but I still need to double-check that.
This comment has been minimized.
This comment has been minimized.
|
I tried to inspect MIR for both versions to get an idea of how much code is generated by each: https://godbolt.org/z/7zv1jqcvT In short, the match version generates a constant, a local and basic block with 5 statements for each variant. One thing that stands out is that each constant in the |
- Add `debug_c_like_enum_write_str` to avoid duplicating similar code
d9870e7 to
5d12b9b
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Implement `Debug` for C-like enums with a concatenated string
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (b5d9a4c): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 4.2%, secondary 5.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%, secondary 1.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 489.287s -> 489.11s (-0.04%) |
View all comments
Fixes: #114106 #133945
Related to: #88793
Continuation of: #109615 #114190
r? @ghost (I want to see the perf first)