Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: separate owning and non-owning AliasedBuffers #33142

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Apr 29, 2020

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
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

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.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 29, 2020
@nodejs-github-bot
Copy link
Collaborator

@@ -10,7 +10,7 @@ void AllocateAndResizeBuffer(
v8::Isolate* isolate = args.GetIsolate();
int64_t length = args[0].As<v8::BigInt>()->Int64Value();

node::AliasedBigUint64Array array{isolate, 0};
node::OwningAliasedBigUint64Array array{isolate, 0};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

class V8T,
// SFINAE NativeT to be scalar
typename Trait = std::enable_if_t<std::is_scalar<NativeT>::value>>
class AliasedBufferViewBase;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jasnell
Copy link
Member

jasnell commented Apr 29, 2020

Agreeing generally with @addaleax on this... would like to know more of the justification behind this.

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 30, 2020

conceptually these AliasedBuffers are like ArrayBufferViews

@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)

@joyeecheung
Copy link
Member Author

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.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@joyeecheung joyeecheung requested review from a team as code owners August 10, 2020 16:07
@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Oct 16, 2020
@github-actions
Copy link
Contributor

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.

@github-actions
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants