Resolver: Introduce CmRef which has a speclative borrow variant for CmRefCell - #160271
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.
Resolver: Introduce `CmRef` which has a speclative borrow variant for `CmRefCell`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (caf2dd1): comparison URL. Overall result: ❌ regressions - 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 (primary 1.5%, secondary 1.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 0.3%, secondary -1.2%)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: 490.333s -> 490.941s (0.12%) |
|
@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.
Resolver: Introduce `CmRef` which has a speclative borrow variant for `CmRefCell`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (14a8f12): comparison URL. Overall result: ❌ regressions - 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 (primary 4.1%, secondary 2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 0.4%, secondary 0.2%)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: 492.106s -> 489.164s (-0.60%) |
|
I'm surprised it's a regression at all, it's such a tiny thing, checking a flag once per module, not even per declaration. |
|
It is also per Resolution, those are wrapped in a CmRefCell as well.
|
I mean in this PR, not in follow ups. I've been trying to use a closure-based interface for this for the last half an hour, like fn with_resolutions<R>(&self, module: Module<'ra>, f: impl FnOnce(&ResolutionTable<'ra>) -> R) -> R { ... }but it's getting too unwieldy, maybe not today. r=me on this PR after squashing commits.
|
|
Reminder, once the PR becomes ready for a review, use |
… the underlying `RefCell`
3a43b26 to
9890f66
Compare
right yeah, forgot the revert.
squashed, but can't |
i'll open a pr based of this pr with the token, rebasing will remove this pr's commit. Edit: Do you want the new version of |
|
@bors r+ rollup |
Yes, the full |
…uwer Rollup of 8 pull requests Successful merges: - #160262 (Library lock file maintenance) - #158548 (Move `std::io::copy` to `alloc::io`) - #158814 (Produce an error when `#[inline]` and `#[rust_force_inline]` are used together) - #160025 (Fix an edge case with `StepBy::nth` on non-fused iterators) - #160271 (Resolver: Introduce `CmRef` which has a speclative borrow variant for `CmRefCell`) - #160281 (Fix(lib/fs/tests): Avoid permission denials when cleaning up TempDirs in `set_get_permissions_nofollows*`) - #160325 (tidy: Check `proc_macro_deps.rs` by reading it, not by including it) - #160334 (Add regression test for unused_allocation on boxed comparison)
Rollup merge of #160271 - LorrensP-2158466:res-cm-refcell, r=petrochenkov Resolver: Introduce `CmRef` which has a speclative borrow variant for `CmRefCell` part of #158845 We now introduce `CmRef` that is returned by `CmRefCell::borrow`, which does a normal borrow of the underlying `T` if we are in speculative resolution. We do not alter state during speculative resolution and because it will be run in parallel in the future, updating the `RefCell` borrow counters is not possible. Thus we just return `&T` that is "untracked". Also allows us to remove the `CmRefCell` wrapper around the external module resolution table. r? @petrochenkov
View all comments
part of #158845
We now introduce
CmRefthat is returned byCmRefCell::borrow, which does a normal borrow of the underlyingTif we are in speculative resolution. We do not alter state during speculative resolution and because it will be run in parallel in the future, updating theRefCellborrow counters is not possible. Thus we just return&Tthat is "untracked".Also allows us to remove the
CmRefCellwrapper around the external module resolution table.r? @petrochenkov