Resolver: add checked methods for Cm(Ref)Cell

[LorrensP-2158466]

View all comments

part of #158845

Introduces extra functions for the mutating functions of Cm(Ref)Cell with a checked suffix that are the same logic as current ones. It allows for the functions without the suffix to ignore the check on the speculative flag by requiring a &mut Resolver, which is only possible if we are definitely not in speculative resolution.

r? @petrochenkov

[LorrensP-2158466]

Ah, one extra thing. The *_with_token methods are not usable in places like this:

// self is &mut Resolver
for _ in &self.iterable_field {
    self.resolution().borrow(self.cm_token()); 
}

because the iterator is shared on the resolver field, doing self.cm_token() is not possible, even if we have a mutable resolver. There isn't really a way to do this safely with the current borrow checker.

[mejrs]

@bors delegate try

[rust-bors]

✌️ @LorrensP-2158466, you can now perform try builds on this pull request!

You can now post @bors try to start a try build.

[LorrensP-2158466]

@bors try @rust-timer queue

[mejrs]

@rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 358e11b (358e11b32135d04ea62b279b8125feaff44464a7)
Base parent: 73dc916 (73dc9167f1cd099e525c9ade2e068d1907b78564)

[rust-timer]

Finished benchmarking commit (358e11b): 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 @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.2%, 0.5%]	16
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.6%, secondary -2.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.4%, 0.5%]	3
Regressions ❌ (secondary)	0.7%	[0.7%, 0.7%]	1
Improvements ✅ (primary)	-3.7%	[-3.7%, -3.7%]	1
Improvements ✅ (secondary)	-6.2%	[-6.2%, -6.2%]	1
All ❌✅ (primary)	-0.6%	[-3.7%, 0.5%]	4

Cycles

Results (primary 0.1%, secondary 2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.4%, 2.0%]	7
Regressions ❌ (secondary)	4.4%	[0.5%, 15.4%]	9
Improvements ✅ (primary)	-0.6%	[-1.0%, -0.4%]	6
Improvements ✅ (secondary)	-1.1%	[-2.3%, -0.5%]	5
All ❌✅ (primary)	0.1%	[-1.0%, 2.0%]	13

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 490.553s -> 490.99s (0.09%)
Artifact size: 390.33 MiB -> 390.43 MiB (0.03%)

[LorrensP-2158466]

It's only unused-warnings, but I am curious what happens when we do our own borrow counting, allowing use to remove the match in CmRef::borrow.

[mejrs]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 5ea1493 (5ea14939f6d4c3e7a2d26981ec58268804b71269)
Base parent: 73dc916 (73dc9167f1cd099e525c9ade2e068d1907b78564)

[petrochenkov]

I wouldn't want to introduce a lot of unsafe code an copypaste of RefCell internals for <0.5% benefits on unused-warnings.
The benchmarks in https://perf.rust-lang.org/compare.html?start=73dc9167f1cd099e525c9ade2e068d1907b78564&end=358e11b32135d04ea62b279b8125feaff44464a7&stat=instructions:u already looks good enough.

I'll review after the second benchmark run finishes and the manual ref counting commit is removed.
@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[LorrensP-2158466]

I wouldn't want to introduce a lot of unsafe code an copypaste of RefCell internals for <0.5% benefits on unused-warnings.

Understandable, I was mostly curious :).

[petrochenkov]

If we have methods taking a token, then panicking methods not taking a token are not necessary anymore (*).
From a mutable resolver we can obtain a token without a dynamic check, from an immutable resolver we can obtain a token with a dynamic check.

impl Resolver {
  fn token1(&mut self) -> CmToken { debug_assert!(!self.assert_speculative); CmToken { ... } }
  fn token2(&self) -> CmToken { assert!(!self.assert_speculative); CmToken { ... } }
}

(*) Unless there are borrow checker issues like #160345 (comment), then we can have more unsafe methods taking a boolean or something.

[rust-timer]

Finished benchmarking commit (5ea1493): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 0.8%, secondary 0.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.3%	[0.4%, 6.9%]	9
Regressions ❌ (secondary)	0.5%	[0.4%, 1.0%]	14
Improvements ✅ (primary)	-3.4%	[-3.4%, -3.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[-3.4%, 6.9%]	10

Cycles

Results (primary 0.3%, secondary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.4%, 1.0%]	7
Regressions ❌ (secondary)	1.5%	[0.5%, 5.3%]	7
Improvements ✅ (primary)	-0.7%	[-0.9%, -0.5%]	3
Improvements ✅ (secondary)	-2.0%	[-3.9%, -0.9%]	5
All ❌✅ (primary)	0.3%	[-0.9%, 1.0%]	10

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 490.553s -> 489.78s (-0.16%)
Artifact size: 390.33 MiB -> 390.45 MiB (0.03%)

[LorrensP-2158466]

From a mutable resolver we can obtain a token without a dynamic check, from an immutable resolver we can obtain a token with a dynamic check.

hmm, lets see, that would be nicer as well.

(*) Unless there are borrow checker issues like

I don't think it would be more "problematic" as it is now.

[LorrensP-2158466]

I actually don't like this way of obtaining a token, I find it rather confusing. I thought of it as a way to circumvent the assert_speculative flag. But now its possible to obtain a CmToken through &self even though you actually have a &mut self.

Also, this is not applicable to CmRefCell::borrow because the flag determines whether we track or untrack a borrow.

@rustbot ready.

[petrochenkov]

I actually don't like this way of obtaining a token, I find it rather confusing. I thought of it as a way to circumvent the assert_speculative flag. But now its possible to obtain a CmToken through &self even though you actually have a &mut self.

I think this can be addressed by making obtaining a token from &self uglier than obtaining a token from &mut self (right now it is the opposite).
Ideally &mut Resolver would just coerce into a CmToken implicitly.
Or just abandon the token entirely and make the cell borrowing methods go in pairs - one with an uglier name taking &Resolver and one with a nicer name taking &mut Resolver. (Having a separate type for the token seems like an over-engineering a bit.)

[LorrensP-2158466]

Or just abandon the token entirely and make the cell borrowing methods go in pairs - one with an uglier name taking &Resolver and one with a nicer name taking &mut Resolver. (Having a separate type for the token seems like an over-engineering a bit.)

Yeah, agreed, something like borrow for &mut Resolver and borrow_checked for &Resolver?

[rustbot]

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.

[LorrensP-2158466]

@rustbot ready