BTreeMap: dig a moat around raw node pointers

[ssomers]

Continues on #78104. Moves BoxedNode and the heart of NodeRef to an island with the lowest layer of abstraction, where all the raw pointer sorcery happens. After many attempts this looks a lot better by moving the node's height along with the pointer

r? @Mark-Simulacrum .

[ssomers]

I would simplify this if #78104 and this PR make it through in the same release.

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[ssomers]

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[Mark-Simulacrum]

I am not sure that the added abstraction here is worth it. It doesn't really abstract away the raw pointer at all -- we still provide and use accessors to the raw contents in almost all places that I can see. Can you elaborate on the goal here?

[Mark-Simulacrum]

I'm not sure why this is called "Unboxed" -- it seems to be equivalent to a NodeRef just without some of the type parameters?

[ssomers]

I'm not sure why BoxedNode is called what it is… it's boxed in the alloc::boxed sense of the word (though it doesn't have to be - the first leaf can in theory be integrated into struct BTreeMap or other wrappers) but in my mind the purpose is to squeeze it into a small box. I would probably call them (BoxedNode and the newcomer) PackedNodePtr and NodePtr, if they came in out of the blue.

[ssomers]

It doesn't really abstract away the raw pointer at all

One may ask why the BoxedNode wrapper exists at all - it doesn't add any type parameters or functionality. Without being in a separate module, it doesn't even ensure that its ptr is only accessed through the interface methods. Whereas NodeRef does a lot of things:

• create, destroy and get the pointer stored in itself and in BoxedNode from/to the actual type;
• offer a mostly internal interface to more safely access the various bits of node data;
• finally, expose high order public functions;
• while at the same time offering static and dynamic typing of itself;
• and various borrow types.

This PR splits off the first layer. Recently I've kind of shielded that layer by putting it into associated functions. Basically, #78222 tried to capture the first two layers at once (in something called NodeRefInner, and additionally abstracting edges so that they exist as a ZST for leaves).

You may say that the first layer is not such a big deal any more, because the raw pointer stored is no longer completely fake since you killed the shared empty root, but when I look at the newcomer, there is enough going on for me to think this is worth while. But it's probably better to move down a few more bits of the higher layer stuff. Maybe all the data access, meaning that the definition of LeafNode and InternalNode moves down too. After all, InternalNode's definition is heavily tied to the way pointers are stored.

[ssomers]

Looking into this again, I'm reminded why it's hard to make this lowest layer do more: access to internal stuff has to be unsafe. Unless you painfully duplicate that data access: unsafe down below, and safe wrappers in NodeRef only for marker::Internal.

Therefore, retreating a little with an earlier version. #79339.

[Mark-Simulacrum]

I don't think I'm seeing any interesting or complicated logic in the new file -- it all looks pretty short and not too "interesting" -- but maybe I'm missing something?

[ssomers]

I think it's interesting to see where and how these two pointers get cast back and forth, in a more narrow scope than the use of a node or ptr field in all of node.rs.

[Mark-Simulacrum]

But none of the complexity in casting is being moved? There's no logic in the new module, it's just hiding the casts behind methods?

[ssomers]

It depends on what you consider to be casting. To me, this PR does move some complexity, the fact we have raw LeafNode pointers and raw InternalNode pointers but we store them in a peculiar way. If it weren't for gdb_providers, this lowest layer could easily eliminate pointer casting from node.rs, because they're only required for packing into and from BoxedNode.

Of course it still leaves the challenge that you get a NodeRef and you have to figure out whether the node is internal, and there is a cast_to_internal method to deal with. But that's orthogonal to how we store an actual pointer to a node.

If you think that layer not valuable, where's the value in BoxedNode? Just a name maybe, and I would agree names are very important, but don't we have `type BoxedNode<K,V> = NonNull<LeafNode<K,V>> for that?

[Mark-Simulacrum]

I would be fine with such a type alias rewrite, yes, I think it's better than this PR probably. My perspective is that the pointer storage is not particularly interesting because it is really quite simple AFAICT.