src: separate owning and non-owning AliasedBuffers - #33142
Conversation
Sorry, something went wrong.
Move the reserve() method into an owning variant of AliasedBuffer, and split the two constructors to different classes to make the contract explicit instead of relying on run time CHECKs. Also deletes unused assignment operator and copy/move constructors to make sure they don't get copied by accident.
Sorry, something went wrong.
| int64_t length = args[0].As<v8::BigInt>()->Int64Value(); | ||
|
|
||
| node::AliasedBigUint64Array array{isolate, 0}; | ||
| node::OwningAliasedBigUint64Array array{isolate, 0}; |
There was a problem hiding this comment.
FWIW (and this is not a blocking comment for this PR) this entire file is currently dead code (it's not executed as part of our tests, see #32626). I'm leaning towards removing it in a future PR (when I get around to it) as it looks like AliasedBuffer tests should be in the cctest and not tested as an addon.
Sorry, something went wrong.
There was a problem hiding this comment.
This seems to artificially introduce a distinction that isn’t really there. AliasedBuffers work like ArrayBufferViews, so I don’t really see what we gain by making ownership of the underlying ArrayBuffer explicit?
Regarding that TODO comment, I took that to mean that the idea would be to create a type of object that would refer to another AliasedBuffer and only contain an additional byte offset or similar, as a more lightweight alternative. (I would also not be a fan of that, because, again, conceptually these AliasedBuffers are like ArrayBufferViews, and have thought about removing the TODO entirely, but given my history with the person who added that comment I didn’t want to seem like I was doing that out of spite or something.)
Sorry, something went wrong.
| class V8T, | ||
| // SFINAE NativeT to be scalar | ||
| typename Trait = std::enable_if_t<std::is_scalar<NativeT>::value>> | ||
| class AliasedBufferViewBase; |
There was a problem hiding this comment.
Either some code comments explaining the differences and why one would be used vs. the other or some information added to src/README.md explaining the same (or both) would be ideal
Sorry, something went wrong.
|
Agreeing generally with @addaleax on this... would like to know more of the justification behind this. |
Sorry, something went wrong.
@addaleax I think the aliased buffer bases are actually just "ArrayBuffers with only one type of view throughout the code base" (for direct use, it could be any type; for being viewed, they are always Uint8), and we make use of the fact that they are only viewed in one specific way to add additional C++ type checks to facilitate casts, and to store the typed array directly so that GetJSArray don't need to generate a view from the underlying buffer. The OwningAliasedBuffer are like "ArrayBuffers with only one type of view throughout the code base, and they can relocate their underlying memory", something that's only made possible by an additional layer of wrapper over the actual v8 construct. The AliasedBufferView are "ArrayBufferViews that peek into a root OwningAliasedUint8Array with their own offsets." They are only allowed to read/write the content of the underlying buffer but they are not allowed to relocate the memory. So I think the distinction is pretty clear, to the point that I didn't really need to try to tell which one is which when I refactored - they invoked different constructors and compiler yelled at me for invoking the owning type of buffer constructor on a non-owning view. And the compiler also yelled at me for invoking reserve() on an AliasedBufferView. I have always seen the class as some sort of union of ArrayBufferViews and TypedArrays that loosely guard two different use cases against each other with CHECKs and overloads, which is somewhat bizarre, because they don't really mix, so might as well just split them into two types. I think we could make some additional cleanup to this, like asserting that owning aliased buffers being viewed by others should not relocate their underlying memory (but those without any other views can) |
Sorry, something went wrong.
|
Also, I think another distinction is that how they are going to be refactored into AliasedStructs. For AliasedBufferViews and the OwningAliasedUint8Array they peek into, it makes more sense to refactor them into AliasedStructs because they are non-homogenous. For owning aliased buffers that aren't viewed by any other aliased buffer views, the refactoring is less relevant because they are just homogenous. The goal should be to eventually only leaving owning aliased buffers that are truly homogenous or are of variable lengths (then they just are not structs) behind and refactor others to AliasedStructs. |
Sorry, something went wrong.
|
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
Sorry, something went wrong.
|
Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions. |
Sorry, something went wrong.
Move the reserve() method into an owning variant of AliasedBuffer,
and split the two constructors to different classes to make the
contract explicit instead of relying on run time CHECKs.
Also deletes unused assignment operator and copy/move
constructors to make sure they don't get copied by accident.
This addresses a TODO in aliased_buffer.h
Checklist