-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
doc: document AliasedBuffers usage scope in style guide #24724
Conversation
CPP_STYLE_GUIDE.md
Outdated
need to be transferred between C++ and JS semantics, and can incur | ||
significant effort if passing around a lot with the data. | ||
|
||
AliasedBuffer on the other had, provides direct access to the backing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AliasedBuffer on the other had, provides direct access to the backing | |
AliasedBuffer on the other hand, provides direct access to the backing |
Um … this reads a bit confusing to me. AliasedBuffers are TypedArrays, and conversely, we can directly access a TypedArray’s memory? I’m not sure there’s anything to actually choose here… The last paragraph also seems somewhat odd, because the reason that we have AliasedBuffer is that we can use the abstraction to hook into the VM’s object modification observation mechanisms; so it seems like the opposite is true? Not using AliasedBuffer to modify TypedArray memory is what breaks these mechanisms. (This detection stuff is only actually supported by @nodejs/chakracore, afaik, and I’m not sure about the state of that project as a whole.) So: We should use |
CPP_STYLE_GUIDE.md
Outdated
@@ -21,6 +21,7 @@ | |||
* [Use explicit pointer comparisons](#use-explicit-pointer-comparisons) | |||
* [Ownership and Smart Pointers](#ownership-and-smart-pointers) | |||
* [Avoid non-const references](#avoid-non-const-references) | |||
* [Choice between Typed Arrays and AliasedBuffers](#typed-arrays-andaliased-buffers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* [Choice between Typed Arrays and AliasedBuffers](#typed-arrays-andaliased-buffers) | |
* [Choice between Typed Arrays and AliasedBuffers](#choice-between-typed-arrays-and-aliasedbuffers) |
CPP_STYLE_GUIDE.md
Outdated
### Choice between Typed Arrays and AliasedBuffers | ||
|
||
Typed Arrays define APIs for handling binary data. While the usage of | ||
these APIs are straightforward, the data created through these APIs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these APIs are straightforward, the data created through these APIs | |
these APIs is straightforward, the data created through these APIs |
CPP_STYLE_GUIDE.md
Outdated
|
||
AliasedBuffer on the other had, provides direct access to the backing | ||
buffer of a JS object. This means direct writes into the buffer from C++ | ||
becomes visible through the wrapping JS object and vice versa. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
becomes visible through the wrapping JS object and vice versa. | |
become visible through the wrapping JS object and vice versa. |
CPP_STYLE_GUIDE.md
Outdated
buffer of a JS object. This means direct writes into the buffer from C++ | ||
becomes visible through the wrapping JS object and vice versa. | ||
|
||
While this is more efficient than Typed Arrays, it overrides the v8's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is more efficient than Typed Arrays, it overrides the v8's | |
While this is more efficient than Typed Arrays, it overrides the V8's |
thanks for the reviews! let me wait to hear from @joyeecheung as well. |
I agree with @addaleax . For the background of the The most applicable case of They are also used when the ArrayBufferViews need to be persistent, e.g. the ones in the |
062d47c
to
240df6e
Compare
thanks for all the reviews! pushed a modified version that I think covers the essence of inputs especially from @addaleax and @joyeecheung ; PTAL, thanks! |
CPP_STYLE_GUIDE.md
Outdated
|
||
// Modify the data through the setters at specified index | ||
// Thus, the setter has a chance to hook mutation observers | ||
data.SetValue(0, 12345); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually use the []
accessors, so I think the style guide should list them as the primary alternative?
CPP_STYLE_GUIDE.md
Outdated
So if the object's backing data is being modified from C++ side | ||
out-of-band with the calling context, the recommended abstraction to use | ||
is AliasedBuffer class, that has place holders to hook observers | ||
for data mutations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph is very hard to read for me.
I don’t want to tell you what to write, but I think something like this would do:
When modifying typed arrays directly from C++, use an
AliasedBuffer
when possible,
because that makes it easier for some JS debuggers to observe these modifications.
We’d probably want to go into more detail when – if ever – something like node-chakracore gets merged into Node.js core, and we actually need to take care of always making modifications observable. For now, the chakracore team has been doing that on their own in the places where we don’t use AliasedBuffer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax - thanks, pushed a modified version with almost your suggested wordings. ptal, thanks.
CPP_STYLE_GUIDE.md
Outdated
|
||
// Modify the data through natural operator semantics, | ||
// but those operators are overloaded in AliasedBuffer | ||
// and provide placeholders to hook mutation observers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this provides placeholders is only relevant to VM engine implementers (i.e. not Node.js coders), so I don’t think it needs to be in the style guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This information is also included in the doc comment for AliasedBuffer
, which is a much better place for it anyway?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, removed the comment that reveals info relevant to VM implementors.
CPP_STYLE_GUIDE.md
Outdated
|
||
When working with typed arrays that involves direct data modification | ||
from C++, and when such modifications can have side effects outside | ||
of the callsite, use an AliasedBuffer when possible, because that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I’m still somewhat confused… What does
when such modifications can have side effects outside of the callsite
mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean to express this:
If the C++ layer modification of data is unanticipated / unforeseen / invisible at the callsite.
Example:
buffer.fill('x')
here, the backend buffer is filled with data at C++ level, and this is evident by looking at the JS callsite. Whereas:
obj.registerModule('foo)';
running into C++ land, and modifying artifacts linked to obj
is not evident at the callsite.
My way of telling is to recommend AliasedBuffer usage in second type of cases. Does it make any meaning to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gireeshpunathil Yes, but … the thing is, afaik (and you or @nodejs/chakracore can correct me if I’m wrong), there’s no real difference between these cases.
The VM debugger wouldn’t be notified of modifications even in the buffer.fill('x')
case; and because we don’t use AliasedBuffer (and we can’t really, unless we port memset/memcpy to it), they still have to do explicit notifications after our modifications:
(They haven’t fully covered all the Buffer
methods, to be fair.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax - ok; I removed that phrase to avoid confusion.
[On a side note]: In a sufficiently large application, what the VM (or a monitoring service that leverages the VM function) observed data at this level will be useful for? In other words, for a program or a user, a buffer.fill
is an obvious callsite that mutates the buffer data at some lower levels, so may not be of real interest (or of any use), but would be interested to get notified on an out-of-bounds access because there is no normal way of knowing it otherwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gireeshpunathil So, the ChakraCore application for this whole notification thingy is time-travel debugging… I haven’t ever tried that myself, but I believe the purpose here is to be able to compare the values of JS objects at different points in time, and that’s what the notifications are for? Without this, I think the debugger could show the same contents of a buffer after a .fill()
call and when going back in time to before that call?
CPP_STYLE_GUIDE.md
Outdated
@@ -21,6 +21,7 @@ | |||
* [Use explicit pointer comparisons](#use-explicit-pointer-comparisons) | |||
* [Ownership and Smart Pointers](#ownership-and-smart-pointers) | |||
* [Avoid non-const references](#avoid-non-const-references) | |||
* [When to use AliasedBuffers](#when-to-use-aliasedbuffers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Use AliasedBuffers to manipulate TypedArrays
would be easier to recognize in the TOC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joyeecheung - done, thanks.
CPP_STYLE_GUIDE.md
Outdated
|
||
When working with typed arrays that involves direct data modification | ||
from C++, use an AliasedBuffer when possible, because that makes it | ||
easier for some JS debuggers to observe these modifications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term some JS debuggers
reads somewhat strange to me, can we just omit the reason and direct the reader to the comment in aliased_buffer.h? (one needs to read the header to use it anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified accordingly, and linked with the header file section for reference. Hope @addaleax is good with this change?
Explain usage context and scope of AliasedBuffer API and its function in the C++ style guide. Provide an example code. Fixes: nodejs#22977
def2c93
to
1917213
Compare
@targos @vsemozhetbyt the content you have reviewed and change suggested is fundamentally changed, so please take another look, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about these suggestions, so it would be good to get a +1 from native speakers and C++ users. Otherwise, doc format LGTM.
CPP_STYLE_GUIDE.md
Outdated
@@ -257,6 +258,21 @@ class ExampleClass { | |||
}; | |||
``` | |||
|
|||
### Use AliasedBuffers to manipulate TypedArrays | |||
|
|||
When working with typed arrays that involves direct data modification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When working with typed arrays that involves direct data modification | |
When working with typed arrays that involve direct data modification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed, thanks.
CPP_STYLE_GUIDE.md
Outdated
### Use AliasedBuffers to manipulate TypedArrays | ||
|
||
When working with typed arrays that involves direct data modification | ||
from C++, use an AliasedBuffer when possible. The API abstraction and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from C++, use an AliasedBuffer when possible. The API abstraction and | |
from C++, use an `AliasedBuffer` when possible. The API abstraction and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
CPP_STYLE_GUIDE.md
Outdated
|
||
When working with typed arrays that involves direct data modification | ||
from C++, use an AliasedBuffer when possible. The API abstraction and | ||
the usage scope of AliasedBuffer is documented in [aliased_buffer.h][]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the usage scope of AliasedBuffer is documented in [aliased_buffer.h][]. | |
the usage scope of `AliasedBuffer` are documented in [aliased_buffer.h][]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
CPP_STYLE_GUIDE.md
Outdated
the usage scope of AliasedBuffer is documented in [aliased_buffer.h][]. | ||
|
||
```c++ | ||
// Create an AliasedBuffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Create an AliasedBuffer | |
// Create an AliasedBuffer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@vsemozhetbyt thanks for the suggestion, and the recommendation. /cc @Trott (a native speaker) and @jasnell (a C++ user / both) - request a review! |
CPP_STYLE_GUIDE.md
Outdated
|
||
When working with typed arrays that involve direct data modification | ||
from C++, use an `AliasedBuffer` when possible. The API abstraction and | ||
the usage scope of `AliasedBuffer` is documented in [aliased_buffer.h][]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more nit:
the usage scope of `AliasedBuffer` is documented in [aliased_buffer.h][]. | |
the usage scope of `AliasedBuffer` are documented in [aliased_buffer.h][]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vsemozhetbyt - changed, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM, yes :)
scheduled CI: https://ci.nodejs.org/job/node-test-pull-request/19195 |
Landed in 447b390 |
Explain usage context and scope of AliasedBuffer API and its function in the C++ style guide. Provide an example code. Fixes: nodejs#22977 PR-URL: nodejs#24724 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Explain usage context and scope of AliasedBuffer API and its function in the C++ style guide. Provide an example code. Fixes: nodejs#22977 PR-URL: nodejs#24724 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Expose AliasedBuffer API and its function through the C++ style guide, and also differentiate it with
TypedArrays.
Fixes: #22977
A draft attempt to follow @joyeecheung 's thought, open to iterate over the verbiage.
/cc @joyeecheung @addaleax
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes