Fix #3846 properly, so that subtrees can be skipped again #4006

JoJoDeveloping · 2024-11-01T14:50:23Z

Issue #3846 was "fixed" by PR #3847, but this fix was a hotfix. After careful thinking about the invariants at play there, the more proper solution is to instead do some extra magic during retags. This PR re-adds the disabled functionality, and ensures that default-lazy new nodes introduced by a retag outside that retag's range do not break anything, by having such retags reset the state that tracks when subtrees can be skipped.

The details can be seen in the code comments.

Here's the difference in running times for the benchmark suite. Note that it's a log graph, since the differences are so large it would otherwise be unreadable:

This commit supplies a real fix, which makes retags more complicated, at the benefit of making accesses more performant.

JoJoDeveloping · 2024-11-01T23:21:17Z

src/borrow_tracker/tree_borrows/tree.rs

+                    // So we return `true` here, which is definitely correct, but maybe not fully optimal.
+                    // But in cases where we are imprecise, the iteration stops at the next initialized parent anyway,
+                    // so it is likely not a huge loss in practice.
+                    None => true,


I'm still somewhat unhappy with this. In cases where a node is mostly default-initialized, this performance loss here can become significant. A better solution might be to track the "default last foreign access" for not-yet-materialized nodes, similar to how we track the default permission for these. And then update that in here.

Indeed, this seems to be the issue in zip-equal where a better handling of this makes things go 28% more brrr.

RalfJung · 2024-11-02T16:18:06Z

Well those are some solid wins. :)

(How did you make those graphs?)

JoJoDeveloping · 2024-11-02T16:19:44Z

(How did you make those graphs?)

google sheets--the numbers were copied in there manually.

Well those are some solid wins. :)

Not really--it's more that #3847 was a solid loss 🙃 this just brings the performance back to where it used to be.

RalfJung · 2024-11-02T16:23:15Z

Not really--it's more that #3847 was a solid loss 🙃 this just brings the performance back to where it used to be.

The previous version of this cheated by being wrong, so it still counts. ;)

src/borrow_tracker/tree_borrows/perms.rs

RalfJung · 2024-11-02T16:25:22Z

src/borrow_tracker/tree_borrows/tree.rs

+    /// which states that a node's children survive accesses at least as strong as that node itself.
+    /// While retags act as a read for the offsets specified in the retag, it is also present at the other
+    /// offsets, but marked as "lazy." Such lazy nodes can, however, still be affected by foreign accesses.
+    /// This would violate the invariants that children always survive accesses at least as strong as the last


Suggested change

/// This would violate the invariants that children always survive accesses at least as strong as the last

/// This would violate the invariant that children always survive accesses at least as strong as the last

RalfJung · 2024-11-02T16:25:46Z

src/borrow_tracker/tree_borrows/tree.rs

+    /// recorded foreign access at the parent.
+    /// To restore the invariant, we need to weaken the information stored in the parent, until it is weak enough
+    /// to encompass the newly added lazy node.
+    /// This function compares two accesses, and indicates if the parents needs to be updated.


should be either "parents need" or "parent needs"

RalfJung · 2024-11-02T16:28:06Z

src/borrow_tracker/tree_borrows/tree.rs

+            // if the last access here was a foreign access, but our new child does not
+            // survive any foreign access, we must let the next foreign access through again


Suggested change

// if the last access here was a foreign access, but our new child does not

// survive any foreign access, we must let the next foreign access through again

// If the last access here was a foreign access (`last_recorded` is `Some`),

// but our new child does not survive any foreign access (`happening_now`,

// is `None`), we must let the next foreign access through again.

RalfJung · 2024-11-02T16:31:31Z

src/borrow_tracker/tree_borrows/tree.rs

+    /// See also `reset_last_foreign_access_after_reborrow`.
+    /// Retags can violate the invariant on `latest_foreign_access`,
+    /// which states that a node's children survive accesses at least as strong as that node itself.
+    /// While retags act as a read for the offsets specified in the retag, it is also present at the other


This same comment is copied 3 times it seems... seems better to instead reference a common pace where this is explained once.

Indeed. But unfortunately Option<AccessKind> has no obvious place where one would add such an annotation.
Note that Option<AccessKind> already has the here-suggested order "natively" since it implements Ord, and it turns out that the auto-derived order is the correct one here (this is not entirely accidental). But relying on that seems wrong, and the code does not do that currently. Perhaps one could introduce a newtype?

But relying on that seems wrong,

Why? Your comments treat the type as having an order. And it does even actually have that order. We should do that.

If you feel uneasy relying on it, add a little #[test] function asserting that the order is the way we want it to be.

RalfJung · 2024-11-02T16:32:38Z

src/borrow_tracker/tree_borrows/tree.rs

+                    // Returns `true` if it changed something, and in that case, we need to continue with the parent.
+                    Some(perm) => perm.reset_last_foreign_access_after_retag(strongest_survivable),
+                    // if there is no permission yet, it is still "default lazy".
+                    // in that case, we would need to compare with that node's original default permission.


Suggested change

// in that case, we would need to compare with that node's original default permission.

// In that case, we would need to compare with that node's original default permission.

RalfJung · 2024-11-02T16:33:53Z

src/borrow_tracker/tree_borrows/tree.rs

+                    // than the `strongest_survivable` by the newly inserted child.
+                    // Returns `true` if it changed something, and in that case, we need to continue with the parent.
+                    Some(perm) => perm.reset_last_foreign_access_after_retag(strongest_survivable),
+                    // if there is no permission yet, it is still "default lazy".


Suggested change

// if there is no permission yet, it is still "default lazy".

// If there is no permission yet, it is still "default lazy".

RalfJung · 2024-11-02T16:34:07Z

src/borrow_tracker/tree_borrows/tree.rs

+            // record whether we did any change (if not, the invariant is restored).
+            let any_change = self.rperms.iter_mut_all().any(|(_, map)| {
+                match map.get_mut(current) {
+                    // if there is a permission, ensure that it's latest recorded foreign access is not stronger


Suggested change

// if there is a permission, ensure that it's latest recorded foreign access is not stronger

// If there is a permission, ensure that its latest recorded foreign access is not stronger

RalfJung · 2024-11-02T16:34:44Z

src/borrow_tracker/tree_borrows/tree.rs

+                    // if there is no permission yet, it is still "default lazy".
+                    // in that case, we would need to compare with that node's original default permission.
+                    // But that depended on whether that node was protected when it was created, which is no
+                    // longer the case now.


I don't understand this sentence. What is no longer the case now?

The protector might have in the meantime gone away. But when it was initially inserted, the "update parent's latest_foreign_access" logic took the protector into account when it reset that field in the parents. So if the protector is now gone (and in general that information is not even piped through to this point in the code here), we get different results for what the "implicit initial weakening" did, and what state the invariant is in. And at that point, the code becomes too complicated to be maintainable 😛

So the point is if the protector disappeared the "strongest idempotent access" might become stronger but currently the code doesn't exploit that?

Thinking about it a bit more, I think that without the protector the strongest idempotent foreign access (SIPA) only becomes stronger. Which means that one could (I think) always compute that SIPA at this point with protector := false, and it's still correct, because it will over-approximate.

without the protector the strongest idempotent foreign access (SIPA) only becomes stronger.

That's easy to test exhaustively.

However, isn't the smaller ("weaker") access the conservative choice? I am confused.

because it's the one that was used to modify that field in the parents, i.e. the one that the invariant relates to. If you now have a new child, you want to make an inference about the parents, whether they're already weak enough for you to stop iterating. So you need to know a lower (or is it upper) bound on the parents, which (in the case of still-to-be-default-initialized fields) is the the thing you used when you inserted it. Which is the SIFA for the permission it had when you inserted it.

Oh this is just about stopping the traversal upwards, not about changing anything in what we store in the current node?

Yes. Currently, the traversal continues. But I'd have hoped one could be smarter here, since this will be hit quite often (basically whenever you only have a reference to a sub-range).

And computing the SIFA based on the current protector state is pointless? Or just not good enough for your perfectionism? ;)

It's also that you'd need to add an extra parameter everywhere to get the current protection state, which would be annoying (in particular since it's only for an imperfect solution)

JoJoDeveloping · 2024-11-19T16:30:00Z

I pushed some quite large changes, which restructure a lot and rewrite most of the comments. Please have a look at it again, starting in the new file foreign_access_skipping.rs 🙂

src/borrow_tracker/tree_borrows/foreign_access_skipping.rs

JoJoDeveloping added 2 commits November 1, 2024 15:37

Add benchmark showing effectivity of subtree skipping

67fac9b

Properly fix rust-lang#3846 by resetting parents on lazy node creation

04630e0

This commit supplies a real fix, which makes retags more complicated, at the benefit of making accesses more performant.

JoJoDeveloping mentioned this pull request Nov 1, 2024

[TB Optimization] Skip subtrees based on the subtree's root node's permissions #4008

Draft

JoJoDeveloping commented Nov 1, 2024

View reviewed changes

RalfJung reviewed Nov 2, 2024

View reviewed changes

RalfJung self-assigned this Nov 4, 2024

refactor foreign access skipping

a7f5fac

JoJoDeveloping force-pushed the tb-fix-3846-retag branch from bbbec3a to a7f5fac Compare November 19, 2024 16:32

JoJoDeveloping commented Nov 19, 2024

View reviewed changes

src/borrow_tracker/tree_borrows/foreign_access_skipping.rs Show resolved Hide resolved

JoJoDeveloping added 2 commits November 19, 2024 21:15

Add mising cfg test

c61eb7f

Address reviews 1: add an assertion to ReservedIM

dff1270

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3846 properly, so that subtrees can be skipped again #4006

Fix #3846 properly, so that subtrees can be skipped again #4006

JoJoDeveloping commented Nov 1, 2024 •

edited

Loading

JoJoDeveloping Nov 1, 2024 •

edited

Loading

JoJoDeveloping Nov 19, 2024

RalfJung commented Nov 2, 2024

JoJoDeveloping commented Nov 2, 2024 •

edited

Loading

RalfJung commented Nov 2, 2024

RalfJung Nov 2, 2024

RalfJung Nov 2, 2024

RalfJung Nov 2, 2024

RalfJung Nov 2, 2024

JoJoDeveloping Nov 2, 2024

RalfJung Nov 2, 2024

RalfJung Nov 2, 2024

RalfJung Nov 2, 2024

RalfJung Nov 2, 2024

RalfJung Nov 2, 2024

JoJoDeveloping Nov 2, 2024 •

edited

Loading

RalfJung Nov 2, 2024

JoJoDeveloping Nov 2, 2024

RalfJung Nov 2, 2024

JoJoDeveloping Nov 2, 2024

RalfJung Nov 4, 2024

JoJoDeveloping Nov 4, 2024

RalfJung Nov 4, 2024

JoJoDeveloping Nov 5, 2024

JoJoDeveloping commented Nov 19, 2024

	/// This would violate the invariants that children always survive accesses at least as strong as the last
	/// This would violate the invariant that children always survive accesses at least as strong as the last

		// if the last access here was a foreign access, but our new child does not
		// survive any foreign access, we must let the next foreign access through again

-            // if the last access here was a foreign access, but our new child does not
-            // survive any foreign access, we must let the next foreign access through again
+            // If the last access here was a foreign access (`last_recorded` is `Some`),
+            // but our new child does not survive any foreign access (`happening_now`,
+            // is `None`), we must let the next foreign access through again.

	// in that case, we would need to compare with that node's original default permission.
	// In that case, we would need to compare with that node's original default permission.

	// if there is no permission yet, it is still "default lazy".
	// If there is no permission yet, it is still "default lazy".

	// if there is a permission, ensure that it's latest recorded foreign access is not stronger
	// If there is a permission, ensure that its latest recorded foreign access is not stronger

Fix #3846 properly, so that subtrees can be skipped again #4006

Are you sure you want to change the base?

Fix #3846 properly, so that subtrees can be skipped again #4006

Conversation

JoJoDeveloping commented Nov 1, 2024 • edited Loading

JoJoDeveloping Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Nov 2, 2024

JoJoDeveloping commented Nov 2, 2024 • edited Loading

RalfJung commented Nov 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoJoDeveloping Nov 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoJoDeveloping commented Nov 19, 2024

JoJoDeveloping commented Nov 1, 2024 •

edited

Loading

JoJoDeveloping Nov 1, 2024 •

edited

Loading

JoJoDeveloping commented Nov 2, 2024 •

edited

Loading

JoJoDeveloping Nov 2, 2024 •

edited

Loading