Avoid zero-sized leaf allocations in BTreeMap
#28497
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
cc @gankro -- who owns this code? This change certainly seems safe to me, but I'm not familiar enough to know if there is a better way to go about it. |
|
There's likely a better way to go about this, at least in terms of memory usage, but this is probably the least invasive way, and the usage of zero-sized keys in this collection is of questionable utility. Additionally, the fix is expected to be short-lived, given that @gereeter's parent pointer rewrite is almost complete. |
When both the key and value types were zero-sized, `BTreeMap` previously called `heap::allocate` with `size == 0` for leaf nodes, which is undefined behavior, and jemalloc would attempt to read invalid memory, crashing the process. This avoids undefined behavior by allocating enough space to store one edge in leaf nodes that would otherwise have `size == 0`. Although this uses extra memory, maps with zero-sized key types that have sensible implementations of the ordering traits can only contain a single key-value pair (and therefore only a single leaf node), and maps with key and value types that are both zero-sized have few uses, if any. Furthermore, this is a temporary fix that will likely be unnecessary once the `BTreeMap` implementation is rewritten to use parent pointers. Closes #28493.
|
Yeah this is just a shim |
|
I'm not a super fan of this, but honestly this is a stupid data structure to make anyway, and I'm just waiting for the rewrite. |
|
@bors r+ |
|
📌 Commit 9526813 has been approved by |
|
@brson this is arguably a regression fix, but this bug has probably been around for ages. I know my originally merged impl didn't have this. |
When both the key and value types were zero-sized, `BTreeMap` previously called `heap::allocate` with `size == 0` for leaf nodes, which is undefined behavior, and jemalloc would attempt to read invalid memory, crashing the process. This avoids undefined behavior by allocating enough space to store one edge in leaf nodes that would otherwise have `size == 0`. Although this uses extra memory, maps with zero-sized key types that have sensible implementations of the ordering traits can only contain a single key-value pair (and therefore only a single leaf node), and maps with key and value types that are both zero-sized have few uses, if any. Furthermore, this is a temporary fix that will likely be unnecessary once the `BTreeMap` implementation is rewritten to use parent pointers. Closes #28493.
When both the key and value types were zero-sized,
BTreeMappreviouslycalled
heap::allocatewithsize == 0for leaf nodes, which isundefined behavior, and jemalloc would attempt to read invalid memory,
crashing the process.
This avoids undefined behavior by allocating enough space to store one
edge in leaf nodes that would otherwise have
size == 0. Although thisuses extra memory, maps with zero-sized key types that have sensible
implementations of the ordering traits can only contain a single
key-value pair (and therefore only a single leaf node), and maps with
key and value types that are both zero-sized have few uses, if any.
Furthermore, this is a temporary fix that will likely be unnecessary
once the
BTreeMapimplementation is rewritten to use parent pointers.Closes #28493.