Replace Box<dyn TraitEngine> with an enum.#155714
Replace Box<dyn TraitEngine> with an enum.#155714nnethercote wants to merge 1 commit intorust-lang:mainfrom
Box<dyn TraitEngine> with an enum.#155714Conversation
|
@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 |
`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.
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%) |
|
☔ The latest upstream changes (presumably #155767) made this pull request unmergeable. Please resolve the merge conflicts. |
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.