Implement Deref(Mut) on BufferVec/UniformVec

[james7132]

This PR fixes #3531, and is required by #3504 to improve rendering performance and readability.

Objective

• Expose more of the Vec API for more efficiently handling larger mutations to the underlying Vec.
• Clean up some parts of the implementation.
• Document the public API of the type.

Solution

• Implemented Deref and DerefMut on BufferVec, targeting the underlying Vec
• Privated the reserve function and renamed to reserve_buffer since it only really needs to run before queuing a write to the buffer.
• Removed all of the pass-through BufferVec functions (is_empty, len, clear, etc), which are now exposed via Deref(Mut).
• Renamed capacity to buffer_capacity to avoid clashing with the Deref'ed capacity from Vec.
• Removed the item_size field from the type as it is run-time constant for the type.
• Documented the new publicly available API for the type.

[james7132]

Added UniformVec to this as well. (Couldn't find a good way to handle DynamicUniformVec).

[superdump]

I’ll do a full review in a bit but capacity can also be removed in my opinion. In reserve we can just use size > self.scratch.len() and otherwise capacity is just self.values.len().

[superdump]

I made a few suggestions for what I think are improvements.

I see what you mean about implementing Deref and DerefMut on DynamicUniformVec. It looks to me like it could be done, but the user needs to know they need to wrap their T in DynamicUniform. I would argue that we should just do that for consistency and deref through to the UniformVec's internal Vec<DynamicUniform<T>>. What do you think?

[james7132]

I see what you mean about implementing Deref and DerefMut on DynamicUniformVec. It looks to me like it could be done, but the user needs to know they need to wrap their T in DynamicUniform. I would argue that we should just do that for consistency and deref through to the UniformVec's internal Vec<DynamicUniform<T>>. What do you think?

I went ahead and did it, though I'm not a fan of exposing the need for users to wrap it themselves, seems like a regression in API design. However, it's probably for the best in terms of consistency here.

[superdump]

Also note the failing docs check.

[superdump]

Looks good to me!

[superdump]

@james7132 could you update this on top of main ready for tomorrow’s round of reviews by Cart?

[alice-i-cecile]

bors try

[bors]

try

Build failed:

• build-wasm (stable, ubuntu-latest)

[superdump]

@cart on #4079 for storage buffers you didn’t want to use deref and derefmut, and instead have accessors so I added .values() and .values_mut(). What do you want to do with this PR/with *UniformVec and BufferVec? They are specifically Vec-like but I think the goal is to expose the internal Vec of values to allow use of all Vec APIs for optimisation purposes. That could be achieved by either deref impls or accessors.

[cart]

My biggest reservation about this is that it is basically saying "vec is our public interface, forever". It is committing to never syncing any internal state between the backing Vec and other fields on UniformVec. For example, maybe in the future we decide we want to preserve data across frames and track what needs to be updated internally. That should probably be a new abstraction, but the point is, exposing the vec internals eliminates all forms of internal state tracking. Likewise, it means we can't choose to abstract out things like the self.values.alloc().init(value) to force callers to use the optimized call (see #3590).

[superdump]

My biggest reservation about this is that it is basically saying "vec is our public interface, forever". It is committing to never syncing any internal state between the backing Vec and other fields on UniformVec. For example, maybe in the future we decide we want to preserve data across frames and track what needs to be updated internally. That should probably be a new abstraction, but the point is, exposing the vec internals eliminates all forms of internal state tracking. Likewise, it means we can't choose to abstract out things like the self.values.alloc().init(value) to force callers to use the optimized call (see #3590).

Should we close this then?

[cart]

Barring further discussion on the matter (I'm open to having my mind changed), then yeah I think we should close this.