Conversation
A secondary factor's root isn't a child of the node above it, so the core zipper's parent lookup unwrapped None. Ask the factor's TrieRef instead.
Brings over the follow-up from fix/product-zipper-is-shared-factor-root. Only report sharing in the last factor, for both ProductZipper and ProductZipperG. In an earlier factor the subtrie below a node continues into the following factors, so the same node reached in different factors is a different subtrie of the product. A cached cata keyed by shared_node_id conflated the two and returned wrong results.
Replaces the last-factor-only rule. Sharing in an earlier factor is still useful (a subtrie grafted twice within one factor is the same subtrie of the product), but the same node reached in two different factors is two different subtries, so its id must depend on the factor. A node id is the node's address. In a canonical address the byte below the top byte equals the top byte (0x00 in user space, 0xff in kernel space), so XOR factor + 1 into that byte: the result is never a canonical address, and no two (factor, node) pairs collide. A hash of the pair would not do in 64 bits. An address with no room reports no sharing, and the last factor keeps the node's own id.
Below a node, the dependent product continues into whatever enroll returns for the path so far and the payload, so the same node reached at two paths can root two different subtries, even within one factor. Tagging the id with the factor, as ProductZipper now does, separates factors but not paths, and the last factor isn't known in advance. The new test grafts one subtrie at [1] and [2] with an enroll that picks the next factor by the first byte; with factor-tagged ids the cached cata reused [1]'s count at [2] and returned 4 instead of 5. Also say at each product zipper's sharing impl why the factor's own id would be inconsistent there.
| if below_top != top { | ||
| return None; | ||
| } | ||
| Some(id ^ ((tag as u64) << 48)) |
There was a problem hiding this comment.
We already use a crate for this which steals bits from alignment, sign, and address space?
There was a problem hiding this comment.
no, we don't use a crate. there's code copied from ointers in src/trie_node.rs:2804.
There was a problem hiding this comment.
In a canonical address the byte below the top byte equals the top
No? For physical addresses you can use the top byte, but for virtual ones you can't do that, and definitely not the top two bytes.
below_top != top
0b00000001 0b00000001 is a perfectly fine start of an untagged pointer.
u8::try_from(factor + 1).ok()?;
Failing sharing on factor count seems devious.
There was a problem hiding this comment.
Failing sharing on factor count seems devious.
of all the limitations that PathMap and MORK have, this one should be the least controversial.
if you have more than 254 factors in a product zipper, please let me know.
doing any sort of traversal there would take too much time.
There was a problem hiding this comment.
there's code copied from ointers in ...
Hmm, why again wasn't this lifted out @luketpeterson ? Seems like we don't want to be doing this ad-hoc, a mistake like this could corrupt the trie.
doing any sort of traversal there would take too much time.
We do traversals over 1000+ bytes all the time
of all the limitations that PathMap and MORK have, this one should be the least controversial.
This should come with a big red exclamation mark. With that, we can move the <=255 assumption to optimize other methods as well perhaps.
There was a problem hiding this comment.
No? For physical addresses you can use the top byte, but for virtual ones you can't do that, and definitely not the top two bytes.
I double-checked myself, and it's system-dependent. for x86, 4K tables, top 2 bytes of virtual memory are either 0x00 or 0xff.
On all systems, top 4 bits are sign-extended. I'll find a better way.
There was a problem hiding this comment.
We do traversals over 1000+ bytes all the time
the limit is on the number of nested maps, not byte depth. so [map1, map2, map3, ... map254]
There was a problem hiding this comment.
I double-checked myself, and it's system-dependent. for x86, 4K tables, top 2 bytes of virtual memory are either 0x00 or 0xff.
Reproduced with custom Alloc on x86, 4k tables, with 5-level paging that 0x0000_0020_0000_0000 and 0x0001_0020_0000_0000 are sent to the same node id, causing trie corruption.
the limit is on the number of nested maps
Unless enrolling factors is slow, search algorithms controlling zippers can definitely still use this, see utils int.
There was a problem hiding this comment.
Converting this to draft until we have a better solution.
If shared node id is u64 and they're essentially pointers, there will be still limits on what we can do with this.
There was a problem hiding this comment.
This strikes me as one of those "difficult in theory but solvable in practice" kind of situations.
- We need to ensure that the factor the node is from is reflected in
_idwithout the possibility of collisions.
We have plenty of bits to give to this use - especially because we can ditch the tags from the ODRC pointers.
So the practical problems are:
- We need a contract that ensures that the inner zipper's _id results never use certain bits, regardless of the impl (currently nothing says _id need to be based on pointers, it's just that using pointers gets us uniqueness for free)
- We need a contract that stops a PZ from wrapping another PZ (something that uses the bits can't then expect the bits to be available for use one level up)
Or, if there is some other really clever digest that can guarantee uniqueness without those limitations, I'm all for it. But I don't know what that would be.
…rvative fix, but it leaves a lot of sharing on the table. This will be revisited in Adam-Vandervorst#136
3 participants
There's an issue with sharing ProductZipper: cannot pass node id, since the same node can be used at different levels.
This is fixed by xoring factor level into the second byte of node id. With a sanity check.
The same approach does not work for DependentProductZipper[G], since a shared node can have different next factor depending on path. Added a test in case someone wants to do it.