Make TB tree traversal bottom-up #3843

JoJoDeveloping · 2024-08-25T15:37:26Z

In preparation for #3837, the tree traversal needs to be made bottom-up, because the current top-down tree traversal, coupled with that PR's changes to the garbage collector, can introduce non-deterministic error messages if the GC removes a parent tag of the accessed tag that would have triggered the error first.

This is a breaking change for the diagnostics emitted by TB. The implemented semantics stay the same.

RalfJung · 2024-08-26T15:35:58Z

src/borrow_tracker/tree_borrows/tree.rs

-    stack: Vec<(UniIndex, AccessRelatedness)>,
+    /// Since the traversal is bottom-up, we need to remember
+    /// whether we're here initially (false) or after visiting all
+    /// children (true). The bool indicates this.


If it is bottom-up, the access would always be after visiting all children, right?

Well, yes, but actually no. There is infrastructure for having the visitation skip entire subtrees if it would not change anything. This is done by remembering the last visit, and if the last visit was a foreign read, and the next visit is a foreign read, then it skips that.

(Note: I just realized that this may be unsound to do in combination with lazy, let me check.. But that's the motivation for visiting everything twice).

Indeed, skipping subtree updates the way we do currently is, eh, wrong and broken 😬 See #3846. We should find a solution to that before we continue here.

And independently of that infrastructure, you must first get to the child to visit. For this you need to walk down.

This comment makes a lot more sense after reading the other comments further down that explain the actual iteration order. :) I wouldn't quite call this "bottom-up".

RalfJung · 2024-08-26T15:38:41Z

src/borrow_tracker/tree_borrows/tree.rs

+            if push_children_of_accessed {
+                let accessed_node = this.nodes.get(accessed_node).unwrap();
+                for &child in accessed_node.children.iter().rev() {
+                    self.stack.push((child, AccessRelatedness::AncestorAccess, false));


Please either use an enum instead of just false here, or make this something like /* after_children */ false or so.

This is not resolved yet -- there's no enum and no comment.

I think an enum would be better.

Ah whoops, I misidentified this line. I thought you meant the argument to that method, not the transient internal element that is pushed onto the encapsulated stack.

I really don't know what to call them, except for Phase1 and Phase2, or maybe BeforeTheRecursiveCallsIfThisWasWrittenAsARecursiveFunction and AfterTheRecursiveCallsIfThisWasWrittenAsARecursiveFunction.

BeforeChildren and AfterChildren?

src/borrow_tracker/tree_borrows/tree.rs

tests/fail/both_borrows/alias_through_mutation.tree.stderr

JoJoDeveloping · 2024-08-26T17:36:46Z

Here's a visualization of this PR walking the nodes. Blue means foreign access, orange means child access, and the accessed node is G (which is why it is in red):

RalfJung · 2024-08-27T11:27:58Z

That's fancy. :-)

The best way to put this in ASCII is probably to label each node with a number, indicating its position in the traversal. Also we can use a smaller tree, it doesn't have to be quite so deep and wide.

bors · 2024-08-27T12:50:54Z

☔ The latest upstream changes (presumably #3847) made this pull request unmergeable. Please resolve the merge conflicts.

In preparation for rust-lang#3837, the tree traversal needs to be made bottom-up, because the current top-down tree traversal, coupled with that PR's changes to the garbage collector, can introduce non-deterministic error messages if the GC removes a parent tag of the accessed tag that would have triggered the error first. This is a breaking change for the diagnostics emitted by TB. The implemented semantics stay the same.

src/borrow_tracker/tree_borrows/tree.rs

RalfJung · 2024-08-27T14:59:23Z

src/borrow_tracker/tree_borrows/tree.rs

                }
            }
        }
+        // Then, handle the accessed node's parent. Here, we need to


Suggested change

// Then, handle the accessed node's parent. Here, we need to

// Then, handle the accessed node's parents. Here, we need to

src/borrow_tracker/tree_borrows/tree.rs

RalfJung · 2024-08-27T15:45:56Z

src/borrow_tracker/tree_borrows/tree.rs

-    /// remember whether we're here initially (false) or after
-    /// visiting all children (true). The bool indicates this.
-    stack: Vec<(UniIndex, AccessRelatedness, bool)>,
+    /// remember whether we're here initially  or after visiting all children.


Suggested change

/// remember whether we're here initially or after visiting all children.

/// remember whether we're here initially or after visiting all children.

RalfJung · 2024-08-27T15:47:19Z

src/borrow_tracker/tree_borrows/tree.rs

+        // so that they are at the bottom of the stack. But later, in `finish_foreign_accesses`
+        // we want to start visiting these children of the accessed node first.
+        // So this method finishes by reversing the stack. This only works if
+        // the stack is empty initially.


Now I am even more confused... we add them first since we want to visit them first, but later we reverse so they aren't actually first then...?

It's a stack. Usually, the things you add first are visited last. Since we visit the accessed node first, it's children would usually go last. This is why we reverse().

RalfJung · 2024-08-27T15:48:20Z

src/borrow_tracker/tree_borrows/tree.rs

    // notes having a child access (nodes 1, 2, 3).
+    //
+    // Finally, remember that the iteration order is not relevant for UB, it only affects
+    // diagnostics. It also affects tree traversal optimizations build on top of this, so


Suggested change

// diagnostics. It also affects tree traversal optimizations build on top of this, so

// diagnostics. It also affects tree traversal optimizations built on top of this, so

RalfJung · 2024-08-27T16:00:09Z

src/borrow_tracker/tree_borrows/tree.rs

+        // Since we go bottom-up, we push the accessed node's children first,
+        // so that they are at the bottom of the stack. But later, in `finish_foreign_accesses`
+        // we want to start visiting these children of the accessed node first.
+        // So this method finishes by reversing the stack. This only works if
+        // the stack is empty initially.


Suggested change

// Since we go bottom-up, we push the accessed node's children first,

// so that they are at the bottom of the stack. But later, in `finish_foreign_accesses`

// we want to start visiting these children of the accessed node first.

// So this method finishes by reversing the stack. This only works if

// the stack is empty initially.

// We want to visit the accessed node's children first.

// However, we will below walk up our parents and push their children (our cousins)

// onto the stack. To ensure correct iteration order, this method thus finishes

// by reversing the stack. This only works if the stack is empty initially.

RalfJung · 2024-08-27T16:01:09Z

src/borrow_tracker/tree_borrows/tree.rs

 impl<'tree> TreeVisitor<'tree> {
-    // Applies `f_propagate` to every vertex of the tree top-down in the following order: first
-    // all ancestors of `start`, then `start` itself, then children of `start`, then the rest.
+    // Applies `f_propagate` to every vertex of the tree in a piecewise bottom-up way: First, visit


Can you make this a doc comment, i.e. make all the // into ///?

Yes, but I would like to point out it wasn't one before.

I saw that. :)

src/borrow_tracker/tree_borrows/tree.rs

JoJoDeveloping · 2024-08-27T16:31:24Z

Help how do I get rustdoc to like my ASCII art 😆

RalfJung · 2024-08-27T17:38:36Z

Looks good! But please squash the history a little. :)

JoJoDeveloping · 2024-08-27T17:41:48Z

But please squash the history a little. :)

Squashed! Let's hope that CI is happy this time

RalfJung · 2024-08-27T17:47:10Z

@bors r+

bors · 2024-08-27T17:47:11Z

📌 Commit 52f08f3 has been approved by RalfJung

It is now in the queue for this repository.

bors · 2024-08-27T17:48:19Z

⌛ Testing commit 52f08f3 with merge 6c0e009...

bors · 2024-08-27T18:13:54Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 6c0e009 to master...

…alfJung Make Tree Borrows Provenance GC compact the tree Follow-up on #3833 and #3835. In these PRs, the TB GC was fixed to no longer cause a stack overflow. One test that motivated it was the test `fill::horizontal_line` in [`tiny-skia`](https://github.com/RazrFalcon/tiny-skia). But not causing stack overflows was not a large improvents, since it did not fix the fundamental issue: The tree was too large. The test now ran, but it required gigabytes of memory and hours of time (only for it to be OOM-killed 🤬), whereas it finishes within 24 seconds in Stacked Borrows. With this merged, it finishes in about 40 seconds under TB. The problem in that test was that it used [`slice::chunked`](https://doc.rust-lang.org/std/primitive.slice.html#method.chunks) to iterate a slice in chunks. That iterator is written to reborrow at each call to `next`, which creates a linear tree with a bunch of intermediary nodes, which also fragments the `RangeMap` for that allocation. The solution is to now compact the tree, so that these interior nodes are removed. Care is taken to not remove nodes that are protected, or that otherwise restrict their children. I am currently only 99% sure that this is sound, and I do also think that this could compact even more. So `@Vanille-N` please also have a look at whether I got the compacting logic right. For a more visual comparison, [here is a gist](https://gist.github.com/JoJoDeveloping/ae4a7f7c29335a4c233ef42d2f267b01) of what the tree looks like at one point during that test, with and without compacting. This new GC requires a different iteration order during accesses (since the current one can make the error messages non-deterministic), so it is rebased on top of #3843 and requires that PR to be merged first.

JoJoDeveloping mentioned this pull request Aug 25, 2024

Make Tree Borrows Provenance GC compact the tree #3837

Merged

JoJoDeveloping force-pushed the tb-bottom-up-iteration branch from eed68ab to bbc5af1 Compare August 25, 2024 15:54

RalfJung reviewed Aug 26, 2024

View reviewed changes

JoJoDeveloping force-pushed the tb-bottom-up-iteration branch from bbc5af1 to 62a3f8e Compare August 27, 2024 14:05

RalfJung reviewed Aug 27, 2024

View reviewed changes

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

RalfJung reviewed Aug 27, 2024

View reviewed changes

src/borrow_tracker/tree_borrows/tree.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 27, 2024

View reviewed changes

Add explanation to TB's "piecewise bottom-up" traversal

52f08f3

JoJoDeveloping force-pushed the tb-bottom-up-iteration branch from eeb0df5 to 52f08f3 Compare August 27, 2024 17:41

bors merged commit 6c0e009 into rust-lang:master Aug 27, 2024

	// Then, handle the accessed node's parent. Here, we need to
	// Then, handle the accessed node's parents. Here, we need to

	/// remember whether we're here initially or after visiting all children.
	/// remember whether we're here initially or after visiting all children.

	// diagnostics. It also affects tree traversal optimizations build on top of this, so
	// diagnostics. It also affects tree traversal optimizations built on top of this, so

Make TB tree traversal bottom-up #3843

Make TB tree traversal bottom-up #3843

Uh oh!

Conversation

JoJoDeveloping commented Aug 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JoJoDeveloping commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Aug 27, 2024

Uh oh!

bors commented Aug 27, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoJoDeveloping commented Aug 27, 2024

Uh oh!

RalfJung commented Aug 27, 2024

Uh oh!

JoJoDeveloping commented Aug 27, 2024

Uh oh!

RalfJung commented Aug 27, 2024 via email

Uh oh!

bors commented Aug 27, 2024

Uh oh!

bors commented Aug 27, 2024

Uh oh!

bors commented Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JoJoDeveloping commented Aug 26, 2024 •

edited

Loading