Replace Box<dyn TraitEngine> with an enum. - #155714
Conversation
|
@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.
Replace `Box<dyn TraitEngine>` with an enum.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (9f7e3b5): 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 -1.7%, secondary 0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.1%, secondary 1.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 493.683s -> 512.943s (3.90%) |
|
Perf is generally good but I'm concerned about the regressions for I'm not 100% sure, but it kinda just looks like changes in inlinining, e.g. some of the |
887f6a0 to
c05dac8
Compare
|
I rebased. Let's try perf again, just to see if anything has changed... @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.
Replace `Box<dyn TraitEngine>` with an enum.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (f2da668): 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 1.6%, secondary 1.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -1.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 487.787s -> 505.793s (3.69%) |
This comment has been minimized.
This comment has been minimized.
c05dac8 to
21d6dfb
Compare
|
@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.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (c387082): 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 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 (secondary 6.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 8.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 486.983s -> 489.68s (0.55%) |
`ObligationCtxt` contains a `Box<dyn TraitEngine>` that lets it switch between trait solvers. Unfortunately, many short-lived `ObligationCtxt`s are created and the allocation cost is non-trivial. This commit introduces an enum `DualFulfillmentCtxt` that can hold an old or new solver context, avoiding the need for the `Box` and making things a bit faster. The change requires some extra `FromSolverError` bounds in a few places. The commit also removes some unnecessary `E: 'tcx` bounds.
b4aab5b to
ad480f7
Compare
|
@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.
Replace `Box<dyn TraitEngine>` with an enum.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (e30cd34): 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 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 (secondary 3.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 487.476s -> 490.134s (0.55%) |
|
@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.
Replace `Box<dyn TraitEngine>` with an enum.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (cdb8b2c): 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 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 (secondary 0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.8%, secondary 1.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 489.717s -> 487.7s (-0.41%) |
|
I'm not happy with the regressions here, and I can't get rid of them even after trying various things. I'm going to give up. At some point the old trait solver will be removed and we can enjoy a nice perf win then. |
View all comments
ObligationCtxtcontains aBox<dyn TraitEngine>that lets it switch between trait solvers. Unfortunately, many short-livedObligationCtxts are created and the allocation cost is non-trivial.This commit introduces an enum
DualFulfillmentCtxtthat can hold an old or new solver context, avoiding the need for theBoxand making things a bit faster.The change requires some extra
FromSolverErrorbounds in a few places. The commit also removes some unnecessaryE: 'tcxbounds.