doc: document AliasedBuffers usage scope in style guide #24724

gireeshpunathil · 2018-11-29T14:28:49Z

Expose AliasedBuffer API and its function through the C++ style guide, and also differentiate it with
TypedArrays.

A draft attempt to follow @joyeecheung 's thought, open to iterate over the verbiage.

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

nodejs-github-bot · 2018-11-29T14:28:52Z

@gireeshpunathil build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1725/pipeline

targos · 2018-11-29T14:35:34Z

CPP_STYLE_GUIDE.md

+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


Suggested change

AliasedBuffer on the other had, provides direct access to the backing

AliasedBuffer on the other hand, provides direct access to the backing

addaleax · 2018-11-29T16:42:15Z

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 AliasedBuffers when we want the VM to be able to observe modifications from the C++ side. That’s pretty it, as far as I know.

vsemozhetbyt · 2018-11-29T16:46:02Z

CPP_STYLE_GUIDE.md

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


Suggested change

* [Choice between Typed Arrays and AliasedBuffers](#typed-arrays-andaliased-buffers)

* [Choice between Typed Arrays and AliasedBuffers](#choice-between-typed-arrays-and-aliasedbuffers)

vsemozhetbyt · 2018-11-29T16:48:45Z

CPP_STYLE_GUIDE.md

+### 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


Suggested change

these APIs are straightforward, the data created through these APIs

these APIs is straightforward, the data created through these APIs

vsemozhetbyt · 2018-11-29T16:51:25Z

CPP_STYLE_GUIDE.md

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


Suggested change

becomes visible through the wrapping JS object and vice versa.

become visible through the wrapping JS object and vice versa.

vsemozhetbyt · 2018-11-29T16:53:33Z

CPP_STYLE_GUIDE.md

+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 


Suggested change

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

gireeshpunathil · 2018-11-30T05:17:17Z

thanks for the reviews! let me wait to hear from @joyeecheung as well.

joyeecheung · 2018-11-30T11:41:57Z

I agree with @addaleax . For the background of the AliasedBuffer abstraction, see #15077

The most applicable case of AliasedBuffer is when you have some global data that you need to pass between C++ and JS out-of-band. For example, the AliasedBuffers on the Environment fs_stats_field_array are globally shared among all the fs.*stat callback wrappers and fs.*statSync implementation, the C++ bindings put the results in those buffers and the JS code would read them with the assumption that during the execution the data in those global buffers are fresh and are exactly what they want. This makes it difficult for ChakraCore to implement the time-travel debugging because there is no way for the VM to observe this mutation, hence the abstraction was introduced. Although, AFAICT, we now always return the buffer or pass it to callbacks before we return to the JS layer, and the JS layer does not actually grab it from the global binding anymore (even though essentially the values are still global per environment, we only pass around a local reference to that global), so in theory the VM should be able to observe the change without this abstraction now.

They are also used when the ArrayBufferViews need to be persistent, e.g. the ones in the FSReqPromise - those are not globally shared among callsites, but they are globals to the V8 isolate, and can be garbage collected after the promise resolves and the JS land is done with them . Although these are also always resolved back to the JS land now so I guess it's not strictly necessary for the VM to observe them through the abstraction, but nonetheless AliasedBuffer serves as a home-grown abstraction for manipulating these global arrays so there is still merit in it.

gireeshpunathil · 2018-11-30T14:19:20Z

thanks for all the reviews! pushed a modified version that I think covers the essence of inputs especially from @addaleax and @joyeecheung ; PTAL, thanks!

addaleax · 2018-11-30T14:27:33Z

CPP_STYLE_GUIDE.md

+
+// Modify the data through the setters at specified index
+// Thus, the setter has a chance to hook mutation observers
+data.SetValue(0, 12345);


We usually use the [] accessors, so I think the style guide should list them as the primary alternative?

addaleax · 2018-11-30T14:30:42Z

CPP_STYLE_GUIDE.md

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


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.

@addaleax - thanks, pushed a modified version with almost your suggested wordings. ptal, thanks.

addaleax · 2018-12-01T17:42:11Z

CPP_STYLE_GUIDE.md

+
+// Modify the data through natural operator semantics,
+// but those operators are overloaded in AliasedBuffer
+// and provide placeholders to hook mutation observers.


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.

(This information is also included in the doc comment for AliasedBuffer, which is a much better place for it anyway?)

ok, removed the comment that reveals info relevant to VM implementors.

addaleax · 2018-12-01T17:42:44Z

CPP_STYLE_GUIDE.md

+
+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


Sorry, but I’m still somewhat confused… What does

when such modifications can have side effects outside of the callsite

mean?

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?

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

https://github.com/nodejs/node-chakracore/blob/38520056313bc747e3ca6dcc7598c2e51394c0ac/src/node_buffer.cc#L575-L583

(They haven’t fully covered all the Buffer methods, to be fair.)

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

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

gireeshpunathil · 2018-12-02T08:50:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/19132/

joyeecheung · 2018-12-02T09:35:23Z

CPP_STYLE_GUIDE.md

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


Maybe Use AliasedBuffers to manipulate TypedArrays would be easier to recognize in the TOC?

@joyeecheung - done, thanks.

joyeecheung · 2018-12-02T09:36:54Z

CPP_STYLE_GUIDE.md

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


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)

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

gireeshpunathil · 2018-12-02T14:18:24Z

@targos @vsemozhetbyt the content you have reviewed and change suggested is fundamentally changed, so please take another look, thanks!

vsemozhetbyt

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.

vsemozhetbyt · 2018-12-02T15:13:06Z

CPP_STYLE_GUIDE.md

@@ -257,6 +258,21 @@ class ExampleClass {
 };
 ```

+### Use AliasedBuffers to manipulate TypedArrays
+
+When working with typed arrays that involves direct data modification


Suggested change

When working with typed arrays that involves direct data modification

When working with typed arrays that involve direct data modification

changed, thanks.

vsemozhetbyt · 2018-12-02T15:13:23Z

CPP_STYLE_GUIDE.md

+### 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


Suggested change

from C++, use an AliasedBuffer when possible. The API abstraction and

from C++, use an `AliasedBuffer` when possible. The API abstraction and

vsemozhetbyt · 2018-12-02T15:13:38Z

CPP_STYLE_GUIDE.md

+
+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][].


Suggested change

the usage scope of AliasedBuffer is documented in [aliased_buffer.h][].

the usage scope of `AliasedBuffer` are documented in [aliased_buffer.h][].

vsemozhetbyt · 2018-12-02T15:14:20Z

CPP_STYLE_GUIDE.md

+the usage scope of AliasedBuffer is documented in [aliased_buffer.h][].
+
+```c++
+// Create an AliasedBuffer


Suggested change

// Create an AliasedBuffer

// Create an AliasedBuffer.

gireeshpunathil · 2018-12-02T16:35:53Z

@vsemozhetbyt thanks for the suggestion, and the recommendation. /cc @Trott (a native speaker) and @jasnell (a C++ user / both) - request a review!

vsemozhetbyt · 2018-12-02T17:18:21Z

CPP_STYLE_GUIDE.md

+
+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][].


One more nit:

Suggested change

the usage scope of `AliasedBuffer` is documented in [aliased_buffer.h][].

the usage scope of `AliasedBuffer` are documented in [aliased_buffer.h][].

@vsemozhetbyt - changed, thanks.

addaleax

Still LGTM, yes :)

gireeshpunathil · 2018-12-04T12:24:39Z

scheduled CI: https://ci.nodejs.org/job/node-test-pull-request/19195

Trott · 2018-12-04T18:27:59Z

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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

Explain usage context and scope of AliasedBuffer API and its function in the C++ style guide. Provide an example code. Fixes: #22977 PR-URL: #24724 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

Explain usage context and scope of AliasedBuffer API and its function in the C++ style guide. Provide an example code. Fixes: #22977 PR-URL: #24724 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 29, 2018

gireeshpunathil requested review from addaleax, joyeecheung and cjihrig November 29, 2018 14:28

targos reviewed Nov 29, 2018

View reviewed changes

vsemozhetbyt reviewed Nov 29, 2018

View reviewed changes

gireeshpunathil force-pushed the cppguide branch from 062d47c to 240df6e Compare November 30, 2018 14:15

addaleax reviewed Nov 30, 2018

View reviewed changes

gireeshpunathil changed the title ~~doc: document choice between TypedArray and AliasedBuffer in style guide~~ doc: document AliasedBuffers usage scope in style guide Dec 1, 2018

addaleax reviewed Dec 1, 2018

View reviewed changes

addaleax approved these changes Dec 2, 2018

View reviewed changes

joyeecheung reviewed Dec 2, 2018

View reviewed changes

doc: add a note on usage scope of AliasedBuffer

1917213

Explain usage context and scope of AliasedBuffer API and its function in the C++ style guide. Provide an example code. Fixes: nodejs#22977

gireeshpunathil force-pushed the cppguide branch from def2c93 to 1917213 Compare December 2, 2018 10:45

joyeecheung approved these changes Dec 2, 2018

View reviewed changes

vsemozhetbyt approved these changes Dec 2, 2018

View reviewed changes

fixup: apply style nits

9960d5e

vsemozhetbyt reviewed Dec 2, 2018

View reviewed changes

fixup: grammar fix

ec0b5f6

addaleax approved these changes Dec 3, 2018

View reviewed changes

Trott closed this Dec 4, 2018

joyeecheung mentioned this pull request Dec 5, 2018

CI failures: 20181201-20181205 nodejs/reliability#19

Closed

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

BethGriggs mentioned this pull request Feb 12, 2019

v10.15.3 proposal #26063

Merged

	AliasedBuffer on the other had, provides direct access to the backing
	AliasedBuffer on the other hand, provides direct access to the backing

	* [Choice between Typed Arrays and AliasedBuffers](#typed-arrays-andaliased-buffers)
	* [Choice between Typed Arrays and AliasedBuffers](#choice-between-typed-arrays-and-aliasedbuffers)

	these APIs are straightforward, the data created through these APIs
	these APIs is straightforward, the data created through these APIs

	becomes visible through the wrapping JS object and vice versa.
	become visible through the wrapping JS object and vice versa.

	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

	When working with typed arrays that involves direct data modification
	When working with typed arrays that involve direct data modification

	from C++, use an AliasedBuffer when possible. The API abstraction and
	from C++, use an `AliasedBuffer` when possible. The API abstraction and

	the usage scope of AliasedBuffer is documented in [aliased_buffer.h][].
	the usage scope of `AliasedBuffer` are documented in [aliased_buffer.h][].

	the usage scope of `AliasedBuffer` is documented in [aliased_buffer.h][].
	the usage scope of `AliasedBuffer` are documented in [aliased_buffer.h][].

doc: document AliasedBuffers usage scope in style guide #24724

doc: document AliasedBuffers usage scope in style guide #24724

Conversation

gireeshpunathil commented Nov 29, 2018

Checklist

nodejs-github-bot commented Nov 29, 2018

Choose a reason for hiding this comment

addaleax commented Nov 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

gireeshpunathil commented Nov 30, 2018

joyeecheung commented Nov 30, 2018 • edited Loading

gireeshpunathil commented Nov 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Dec 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Dec 2, 2018

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Dec 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

gireeshpunathil commented Dec 4, 2018

Trott commented Dec 4, 2018

vsemozhetbyt Nov 29, 2018 •

edited

Loading

joyeecheung commented Nov 30, 2018 •

edited

Loading