perf: Reuse the Elaborator used in outlives/ty.rs #67840
Conversation
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 9d3525cb9e193ff6622382ca97d0ff5ccfe00a6f with merge 1914befff0a7286add08d15ce8bd162c479d12ea... |
|
☀️ Try build successful - checks-azure |
|
Queued 1914befff0a7286add08d15ce8bd162c479d12ea with parent 30ddb5a, future comparison URL. |
|
Storing it directly in The need for |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit ef010deb93eaa3a1b54b77662e08a3a50b0a37be with merge 5d370d4923feb2e292d66958a4fba07d6b95e788... |
perf: Avoid re-interning types in outlives checking In profiling `intern_ty` is a very hot function (9% in the test I used). While there does not seem to be a way to reduce the cost of calling we can avoid the call in some cases. In outlives checking `ParamTy` and `ProjectionTy` are extracted from the `Ty` value that contains them only to later be passed as an argument to `intern_ty` again later. This seems to be happening a lot in my test with `intern_ty` called from outlives is at ~6%. Since all `ParamTy` and `ProjectionTy` are already stored in a `Ty` I had an idea to pass around a `View` type which provides direct access to the specific, inner type without losing the original `Ty` pointer. While the current implementation does so with some unsafe to let the branch be elided on `Deref`, it could be done entirely in safe code as well, either by accepting the (predictable) branch in `Deref` or by storing the inner type in `View` as well as the `Ty`. But considering that the unsafe is trivial to prove and the call sites seem quite hot I opted to show the unsafe approach first. Based on #67840 (since it touches the same file/lines) Commits without #67840 https://github.com/rust-lang/rust/pull/67899/files/77ddc3540e52be4b5bd75cf082c621392acaf81b..b55bab206096c27533120921f6b0c273f115e34a
|
☀️ Try build successful - checks-azure |
|
Queued 5d370d4923feb2e292d66958a4fba07d6b95e788 with parent 7785834, future comparison URL. |
|
Finished benchmarking try commit 5d370d4923feb2e292d66958a4fba07d6b95e788, comparison URL. |
|
☔ The latest upstream changes (presumably #67886) made this pull request unmergeable. Please resolve the merge conflicts. |
Profiling the build of https://github.com/Marwes/combine shows that ~1.3% of the time were spent resizing and rehashing the hash set used in `Elaborator`. By reusing the `Elaborator` between calls this should be mostly eliminated. It does result in some awkwardness since the `RefCell` needs to be borrowed in the returned `Iterator` but it works fine for the current code.
Avoids the risk of attempting to borrow `elaborator` twice by accident. The need for `Captures2` is odd, but it seems like `impl Trait` kept generating lifetime constraints that forced `'a` or `'cx` to be identical to `'tcx` otherwise (due to `'tcx` being invariant I think).
|
Moved |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 365ccde with merge afcb9992e87f7467f8a42fcada070f570cdb9327... |
|
☀️ Try build successful - checks-azure |
|
Queued afcb9992e87f7467f8a42fcada070f570cdb9327 with parent ebbb2bf, future comparison URL. |
|
Finished benchmarking try commit afcb9992e87f7467f8a42fcada070f570cdb9327, comparison URL. |
perf: Avoid re-interning types in outlives checking In profiling `intern_ty` is a very hot function (9% in the test I used). While there does not seem to be a way to reduce the cost of calling we can avoid the call in some cases. In outlives checking `ParamTy` and `ProjectionTy` are extracted from the `Ty` value that contains them only to later be passed as an argument to `intern_ty` again later. This seems to be happening a lot in my test with `intern_ty` called from outlives is at ~6%. Since all `ParamTy` and `ProjectionTy` are already stored in a `Ty` I had an idea to pass around a `View` type which provides direct access to the specific, inner type without losing the original `Ty` pointer. While the current implementation does so with some unsafe to let the branch be elided on `Deref`, it could be done entirely in safe code as well, either by accepting the (predictable) branch in `Deref` or by storing the inner type in `View` as well as the `Ty`. But considering that the unsafe is trivial to prove and the call sites seem quite hot I opted to show the unsafe approach first. Based on #67840 (since it touches the same file/lines) Commits without #67840 https://github.com/rust-lang/rust/pull/67899/files/77ddc3540e52be4b5bd75cf082c621392acaf81b..b55bab206096c27533120921f6b0c273f115e34a
JohnCSimon
added
S-waiting-on-author
|
Ping from triage: @Marwes can you please address the merge conflicts? |
|
The latest perf run still indicates a regression and no improvement. Is there a way to fix that or should this be closed? |
|
Closing. Will try to figure out the regressions and resubmit |
The first commit mostly cleans up some unnecessary allocations (I wouldn't expect it to change runtime noticeably).
The second commit should improve things a bit however.
Profiling the build of https://github.com/Marwes/combine shows that
~1.3% of the time were spent resizing and rehashing the hash set used in
Elaborator. By reusing theElaboratorbetween calls this shouldbe mostly eliminated.
It does result in some awkwardness since the
RefCellneeds to beborrowed in the returned
Iteratorbut it works fine for the currentcode.