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

src/borrow_tracker/tree_borrows/tree.rs

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

RalfJung · 2024-11-25T10:27:33Z

src/borrow_tracker/tree_borrows/foreign_access_skipping.rs

+/// the entire range, which can violate this invariant without explicitly causing a local access which would reset things.
+/// So this needs to be done manually, thus `ensure_no_stronger_than`, and also the test `permission_sifa_is_correct` in `perms.rs`
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Default)]
+pub enum LastForeignAccess {


So is this the "last foreign access" or the SIFA? You seem to be using these two terms interchangeably. That's confusing, please pick one and use it consistently.

I felt uncomfortable with using strongest_idempotent_foreign_access in field names, can I shorten it to sifa? Apart from that, the field used to be called latest_foreign_access but has not changed its semantics. So you're saying it should have had a different name already a year ago?

I'm not interested in discussing which name it should have had a year ago. ;)

The "strongest" part isn't actually relevant for correctness, is it? So IMO "idempotent_foreign_access" should suffice. "local access" doesn't make sense as a variant name, so I would propose

enum IdempotentForeignAccess { None, Read, Write }

src/borrow_tracker/tree_borrows/foreign_access_skipping.rs

src/borrow_tracker/tree_borrows/perms.rs

RalfJung · 2024-11-25T10:34:42Z

src/borrow_tracker/tree_borrows/perms.rs

+                let access = perm.strongest_idempotent_foreign_access(prot);
+                // We now assert it is idempotent, and never causes UB.
+                // First, if the SIFA includes foreign reads, assert it is idempotent under foreign reads.
+                if LastForeignAccess::ForeignRead <= access {


Suggested change

if LastForeignAccess::ForeignRead <= access {

if access >= LastForeignAccess::ForeignRead {

same below

RalfJung · 2024-11-25T10:35:12Z

src/borrow_tracker/tree_borrows/perms.rs

+                // We now assert it is idempotent, and never causes UB.
+                // First, if the SIFA includes foreign reads, assert it is idempotent under foreign reads.
+                if LastForeignAccess::ForeignRead <= access {
+                    assert_eq!(perm, transition::perform_access(AccessKind::Read, AccessRelatedness::DistantAccess, perm, prot).unwrap());


What's a "DistantAccess"? Is that the same as "foreign"? If yes, we should rename. (Can be a separate PR.)

no, it's a cousin access. The AccessRelatedness enum has 4 constructors (child, self, cousin, parent), but for almost all cases, two of them (child+self, cousin+parent) are treated the same. The only exception is in protector end writes.

But here we need to choose a concrete inhabitant. We could also pick Parent, i.e. a different member of the same equivalence class.

Ah, we should rename to CousinAccess then.

And please add a comment here explaining what you just said.

src/borrow_tracker/tree_borrows/tree/tests.rs

RalfJung · 2024-11-25T10:39:50Z

src/borrow_tracker/tree_borrows/tree.rs

-    latest_foreign_access: Option<AccessKind>,
+    /// See `foreign_access_skipping.rs`
+    /// Stores the strongest idempotent foreign access for this location.
+    /// Invariant is that at all children, the strongest idempotent foreign access is stronger (greater).


This is an approximation, right? It's okay for this to always be LocalAccess even if it would be idempotent under foreign reads? That should be documented.

It's always OK for this field to be LocalAccess. Also for the one in Node. That would be as if this whole optimization did not exist.

Okay, then please explain that. Currently the definition of SIFA makes it sound like an exact bound.

It's the strongest it can be because the stronger it is the more subtrees we skip -> the faster it is

JoJoDeveloping · 2024-11-27T14:55:13Z

better now^^?

JoJoDeveloping · 2024-11-28T01:07:21Z

@rustbot ready

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 Outdated 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

RalfJung reviewed Nov 25, 2024

View reviewed changes

RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Nov 25, 2024

address a bunch of reviews

f71894f

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Nov 28, 2024

JoJoDeveloping added 2 commits November 28, 2024 02:08

some more minor renamings

844953b

it's -> its

a6b2cb0

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

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

RalfJung Nov 25, 2024

JoJoDeveloping Nov 26, 2024

RalfJung Nov 26, 2024

RalfJung Nov 25, 2024

RalfJung Nov 25, 2024

JoJoDeveloping Nov 26, 2024

RalfJung Nov 26, 2024

RalfJung Nov 25, 2024

JoJoDeveloping Nov 26, 2024

RalfJung Nov 26, 2024

JoJoDeveloping Nov 26, 2024

JoJoDeveloping commented Nov 27, 2024 •

edited

Loading

JoJoDeveloping commented Nov 28, 2024

	if LastForeignAccess::ForeignRead <= access {
	if access >= LastForeignAccess::ForeignRead {

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

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

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

Choose a reason for hiding this comment

JoJoDeveloping commented Nov 27, 2024 • edited Loading

JoJoDeveloping commented Nov 28, 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

JoJoDeveloping commented Nov 27, 2024 •

edited

Loading