Use the capacity from the entities Vec to initialize Table columns

[james7132]

Objective

When working with realloc, it's a safety invariant to pass in the existing layout of the allocation that is being reallocated. This may not be the case with newly created Tables. Vec::with_capacity's documentation states that it will return an allocation with enough space for at least capacity elements, not exactly capacity. This means that entities.capacity() may be greater than the provided capacity. As the ThinColumns use this as their capacity, the new Layout fed to realloc will not match the allocation originally provided to alloc. This is unsound.

While investigating this, I also found that we were not validating that the total capacity of BlobArray's layout upon reallocation were less than isize::MAX via array_layout_unchecked.

Solution

Begin Table construction by allocating the entities Vec, and use it's capacity to allocate the columns instead of directly feeding the provided capacity into ThinColumn::with_capacity.

Replace the array_layout_unchecked call with a safe call to array_layout, and panic if it fails.

Testing

Tested this locally against existing unit tests and miri.

[SkiFire13]

Nit: could this invariant be better documented on Table's definition, each Table creation and Table's realloc?

[hymm]

Am I understanding this comment correctly that currently Vec::with_capacity would always match the capacity requested, but if the allocator changed then it could potentially allocate more space than requested? https://stdrs.dev/nightly/x86_64-unknown-linux-gnu/src/alloc/raw_vec.rs.html#192. Then would this potentially be a problem with how we're allocating the ThinColumn too?

[james7132]

Then would this potentially be a problem with how we're allocating the ThinColumn too?

Unless they change the safety invariants of already established stable APIs, I don't think so. The safety invariant states it just needs to be called with the same Layout that was used to allocate it, and that's what we're already doing / fixing with this PR.

[james7132]

Upon further inspection, it seems like we weren't actually validating one of the safety invariants of array_layout_unchecked in BlobArray::realloc.

Added the comments @SkiFire13 mentioned.

[SkiFire13]

Look good, just left a couple of comments for some typos.