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.

Sign up for GitHub

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

Jump to bottom

v8 module: allow TypedArray and DataView #23953

Merged
merged 1 commit into from
Oct 31, 2018
Merged

v8 module: allow TypedArray and DataView #23953

merged 1 commit into from
Oct 31, 2018

Conversation

oyyd
Copy link
Contributor

@oyyd oyyd commented Oct 29, 2018

This commit allow passing TypedArray and DataView to:

  • v8.deserialize()
  • new v8.Deserializer()
  • v8.serializer.writeRawBytes()

Refs: #1826

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 nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 29, 2018
@oyyd oyyd mentioned this pull request Oct 29, 2018
Closed
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 29, 2018
@vsemozhetbyt vsemozhetbyt added the v8 engine Issues and PRs related to the V8 dependency. label Oct 29, 2018
@vsemozhetbyt
Copy link
Contributor

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

refack
refack suggested changes Oct 29, 2018
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Nits made me ask a question about timing.

test/parallel/test-v8-serdes.js Outdated
const length = des.readUint32();
const buf = des.readRawBytes(length);

let length = des.readUint32();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please const

test/parallel/test-v8-serdes.js Outdated
assert.strictEqual(buf.toString(), 'hostObjectTag');
for (let i = 0; i < arrayBufferViewsLength; i += 1) {
length = des.readUint32();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these be const length2 and const buf2 please?
Recycling variables should be avoided.

test/parallel/test-v8-serdes.js Outdated
// `v8.serializer.writeRawBytes()` should support both `TypedArray`
// and `DataView`.
const arrayBufferViews = common.getArrayBufferViews(buf);
arrayBufferViewsLength = arrayBufferViews.length;
Copy link
Contributor

@refack refack Oct 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Could this be const arrayBufferViewsLength instead.
.
.
.
Which made be think isn't there a race here?
Is the callbacks guaranteed to be in order?
At minimum this should be documented in a comment

Copy link
Contributor Author

@oyyd oyyd Oct 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Is the callbacks guaranteed to be in order?

Yes. We have only one HostObject so that the callback will be called only once.

I think v8.Serializer.writeRawBytes() being expected to be called inside of the callback of _writeHostObject makes the test a bit confusing. I'm going to refactor this.

@refack
Copy link
Contributor

refack commented Oct 29, 2018

@oyyd as alway thank you for the contribution. While reviewing the code and looking at cases of variable recycling I found that I don't understand the operation order guarantee.
IMHO we should find a way to explicitly test for that, and document that.

refack
refack reviewed Oct 29, 2018
View reviewed changes
test/parallel/test-v8-serdes.js Outdated
@@ -66,6 +66,7 @@ const deserializerTypeError =
}

{
let arrayBufferViewsLength;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe simplest solution is to add a sentinel:

  let has_writeHostObjectCalled = false;

the in _writeHostObject

  has_writeHostObjectCalled = true;

the first line of _readHostObject

  assert.ok(has_writeHostObjectCalled);

Copy link
Contributor

@refack refack Oct 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

or dual use arrayBufferViewsLength - initialise it to false, and then assert

  des._readHostObject = common.mustCall(() => {
    assert.notStrictEqual(false, arrayBufferViewsLength);
...

@oyyd
Copy link
Contributor Author

oyyd commented Oct 30, 2018

@refack Thanks for your advice! Comments addressed.

I have separated the test for v8.Serializer.writeRawBytes() so that we can avoid recycling variables and the tests for TypedArray and DateView should be clearer. I have also added an assertion to ensure that _writeHostObject is called before _readHostObject.

@refack
Copy link
Contributor

refack commented Oct 30, 2018

@refack Thanks for your advice! Comments addressed.

Looks 💯. Thank you for following up!

@refack
Copy link
Contributor

refack commented Oct 30, 2018

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

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 30, 2018
@oyyd @refack
lib: enable TypedArray and DataView for the v8 module
56881b0 
This commit allow passing `TypedArray` and `DataView` to:
- v8.deserialize()
- new v8.Deserializer()
- v8.serializer.writeRawBytes()

PR-URL: nodejs#23953
Refs: nodejs#1826
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack
Copy link
Contributor

refack commented Oct 31, 2018 •
edited
Loading

Landed in 56881b0 🎉

@refack refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 31, 2018
@refack refack merged commit 56881b0 into nodejs:master Oct 31, 2018
targos pushed a commit that referenced this pull request Nov 2, 2018
@oyyd @targos
lib: enable TypedArray and DataView for the v8 module
55adc25 
This commit allow passing `TypedArray` and `DataView` to:
- v8.deserialize()
- new v8.Deserializer()
- v8.serializer.writeRawBytes()

PR-URL: #23953
Refs: #1826
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Nov 14, 2018
Merged
This was referenced Nov 15, 2018
Closed
Closed
Closed
Closed
Closed
Closed
BethGriggs pushed a commit that referenced this pull request Apr 8, 2019
@oyyd @BethGriggs
lib: enable TypedArray and DataView for the v8 module
43f41be 
This commit allow passing `TypedArray` and `DataView` to:
- v8.deserialize()
- new v8.Deserializer()
- v8.serializer.writeRawBytes()

PR-URL: #23953
Refs: #1826
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@oyyd @vsemozhetbyt @refack @bnoordhuis @jasnell @nodejs-github-bot