CAD97 / CAD97/storages-api

Streamlining the API further

Open
#6 6 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
10
Forks
1
PR merge metrics
No merged PRs in 30d

Description

I was taking a fresh look at this API, with an eye towards simplification and unification with the `Allocator` API where it makes sense, and I think there's further potential in this area.

# Resolve handles to NonNull

The API can be simplified by going from `&[MaybeUninit]` and `&mut [MaybeUninit]` to `NonNull<[MaybeUninit]>` instead. Specifically:

- `Storage::resolve_mut` is now unnecessary.
- `MultipleStorage::resolve_many_mut` is now unnecessary, and possibly `MultipleStorage` itself is now unnecessary.
- `SharedMutabilityStorage::resolve_raw` is now unnecessary, and possibly `SharedMutabilityStorage` itself is now unnecessary.

Taken to the extreme, this leaves only 2 traits:

- `Storage`, without its `resolve_mut`.
- `PinningStorage`.

A drastic simplification from the 4 traits currently offered.

# Simplify resolve

I am also considering whether `resolve` truly needs to return a _slice_ rather than just a pointer.

My main concern here is performance, rather than ergonomics. That is, if we take the case of the `Allocator` adapter, then the `resolve` method should ideally be a no-op: `Self::Handle` being a `NonNull`, it would return its argument. This would ensure that the `RawBox` or `RawVec` instantiated with an `Allocator` suffer no performance overhead compared to the current `Box` and `Vec` implementations.

This is especially egregious in the case of `RawBox` where a `RawBox` of a trait object requires reading the metadata off the virtual table -- potentially incurring a cache-miss -- to produce a `Layout`, which is used to produce a slice, which is used to produce a pointer to the first element of the slice, discarding the size we took such pains to obtain[^1].

As such, I would recommend switching to:

```rust
pub unsafe trait Storage {
fn resolve(&self, handle: Self::Handle) -> NonNull;
}
```

And should the user wish for a slice, they can take it from there.

This does assume that the alignment is unnecessary in `resolve`, which means either encoding the alignment into the handle or failing to allocate over-aligned layouts. This seems an acceptable trade-off to me.

[^1] _In an ideal world, the optimizer would see right through all that, after inlining `resolve`, and completely discard the computation of the slice, the computation of the layout, and thus the read of the virtual table. Such an optimization would likely require creating the layout with an unchecked (unsafe) method to avoid the potential of panic throwing off the optimizer, and even then... those things are capricious._

# Do not over-constrain handles

At the moment, handles are quite constrained, and I am not quite sure about the sensibility of imposing `Ord`, `Hash`, `Send`, or `Sync`. I am quite partial to `Copy`, though I could be convinced to shave off even that.

The thing is, a user can always _add_ requirements with a `where` clause, but cannot ever _remove_ any requirement even when they have no need of it.

As such, I would recommend only requiring `Copy`, and leaving off everything else. For example, this would mean:

```rust
unsafe impl Send for RawVec
where
T: Send,
S: Storage + Send,
S::Handle: Send,
{}
```

Which is a tad more verbose, but ultimately more flexible. Given how low-level the `Storage` API intends to be, lower-level than `Allocator` even, verbosity for flexibility seems like the right choice here.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.