Resolver cm token

[LorrensP-2158466]

View all comments

part of #158845
follow up of #160271 (first commit of this pr)

Introduces a CmToken obtainable only through a &mut Resolver. Which allows *_with_token methods to be used in CmCell and CmRefCell mutating functions.

Also changes CmRefCell::borrow to return a CmRef bassed on resolver.assert_speculative, a borrow_with_token is also introduced.

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 think we could shift the safety responsibility to the assert_speculative flag.
Like "you cannot change the value of assert_speculative if any RefOrMuts obtained with Resolver::cm(_mut) are alive, or any CmRefs obtained with CmRefCell::borrow are alive, or [add the full list]".

We could make setting the assert_speculative flag formally unsafe by hiding it into some private field and exposing the modification through an unsafe method.

Some of the borrow methods could actually ensure that their return values are dead statically - if they capture the resolver reference (even through phantom data), then you won't be able to change assert_speculative because changing it requires exclusive access to the resolver. (Although it may be inconvenient to work with.)

[LorrensP-2158466]

Yeah, I like the idea of making assert_speculative an unsafe field.

Some of the borrow methods could actually ensure that their return values are dead statically - if they capture the resolver reference (even through phantom data), then you won't be able to change assert_speculative because changing it requires exclusive access to the resolver. (Although it may be inconvenient to work with.)

Ooh, a sort of guard that ensure the borrow is dropped before being allowed to change assert_speculative. I think we will see some issues with functions that use take a &mut Resolver (view types/borrows would be really handy :D).

I'll do these in separate commits, one for unsafe field and one for tying the borrow to the resolver/unsafe field. Or a another pr?

[petrochenkov]

Another PR, I'm busy this week and will be on vacation next week, so reviewing in small pieces will be more convenient.

[LorrensP-2158466]

Okey, i'll work on top of this one.

No, not really needed. i'll just pull this change of tracked/untracked borrows in a separate pr.

[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?