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

util: Add format for SharedArrayBuffer #8587

Closed
wants to merge 1 commit into from
Closed

util: Add format for SharedArrayBuffer #8587

wants to merge 1 commit into from

Conversation

yosuke-furukawa
Copy link
Member

@yosuke-furukawa yosuke-furukawa commented Sep 17, 2016 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

util

Description of change

SharedArrayBuffer is a new global object to share memories between workers.
Currently workers does not work in node, but future v8 has those new global objects.
This change supports a format for SharedArrayBuffer.

Note: this PR is from code-and-learn
nodejs/code-and-learn#56 (comment)

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module. labels Sep 17, 2016
@yosuke-furukawa yosuke-furukawa mentioned this pull request Sep 17, 2016
Closed
4 tasks
@Fishrock123
Copy link
Contributor

Shouldn't we wait for this feature to become publicly available in V8 first?

addaleax
addaleax reviewed Sep 17, 2016
View reviewed changes
lib/util.js
} else if (binding.isSharedArrayBuffer(value)) {
braces = ['{', '}'];
keys.unshift('byteLength');
visibleKeys.byteLength = true;
Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion but I think you can join this with the isArrayBuffer case using ||.

src/node_util.cc
@@ -28,7 +28,8 @@ using v8::Value;
V(isRegExp, IsRegExp) \
V(isSet, IsSet) \
V(isSetIterator, IsSetIterator) \
V(isTypedArray, IsTypedArray)
V(isTypedArray, IsTypedArray) \
V(isSharedArrayBuffer, IsSharedArrayBuffer)
Copy link
Member

Choose a reason for hiding this comment

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

Note: If you rebase against master, this one should already be in the list due to #8510. Feel free to sort that in alphabetically, though. :)

test/parallel/test-util-format-shared-arraybuffer.js
uint8Array[i] = 'a';
}

assert.equal(util.format(sab), 'SharedArrayBuffer { byteLength: 4 }');
Copy link
Member

Choose a reason for hiding this comment

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

assert.strictEqual?

@addaleax
Copy link
Member

Shouldn't we wait for this feature to become publicly available in V8 first?

Well, we’ve already done this for SIMD, and I think it’s okay to be prepared for a release where SABs work without --harmony flags.

@yosuke-furukawa
Copy link
Member Author

Thank you for your review, I fixed them.

@benjamingr
Copy link
Member

I think we should wait until we have a strategy related to SharedArrayBuffer first. @petkaantonov worked on a PR that would enable just that (using SharedArrayBuffer like that) but afaik it is stalled.

alistairjcbrown
alistairjcbrown reviewed Sep 19, 2016
View reviewed changes
lib/util.js
@@ -449,7 +449,7 @@ function formatValue(ctx, value, recurseTimes) {
}
// Fast path for ArrayBuffer. Can't do the same for DataView because it
// has a non-primitive .buffer property that we need to recurse for.
Copy link

Choose a reason for hiding this comment

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

Worth updating this comment too to include reference to SharedArrayBuffer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

src/node_util.cc
@@ -29,6 +28,7 @@ using v8::Value;
V(isRegExp, IsRegExp) \
V(isSet, IsSet) \
V(isSetIterator, IsSetIterator) \
V(isSharedArrayBuffer, IsSharedArrayBuffer) \
Copy link

Choose a reason for hiding this comment

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

Looks like this changes in this file can be removed (it's just moving the line).

Copy link
Member Author

Choose a reason for hiding this comment

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

I just sort the line in alphabetical order.

test/parallel/test-util-format-shared-arraybuffer.js
const util = require('util');

// Pacify eslint.
const SharedArrayBuffer = global.SharedArrayBuffer;
Copy link

Choose a reason for hiding this comment

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

I don't know if there's a preference, but we can use eslints global comment to pacify too

/* global SharedArrayBuffer */

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@yosuke-furukawa
Copy link
Member Author

@benjamingr
I think --harmony option features like SharedArrayBuffer, SIMD are experimental, but we have already related features ( #8510, #7864 ).

@benjamingr
Copy link
Member

@yosuke-furukawa and what exactly would one do with a SharedArrayBuffer in Node? The underlying cluster implementation does not support it.

@alistairjcbrown
Copy link

alistairjcbrown commented Sep 19, 2016 •
edited
Loading

@benjamingr Does that matter for this PR? The data structure is available in node (behind the v8 flag), and when used, the inspect output does not mirror that of the ArrayBuffer. Adding this functionality to the inspect util now means that, when it is being more actively used, more useful information is being output.

$ node --harmony_sharedarraybuffer
> new ArrayBuffer(4)
ArrayBuffer { byteLength: 4 }
> new SharedArrayBuffer(4)
SharedArrayBuffer {}

jasnell
jasnell approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

I'm good with adding this now. LGTM

addaleax
addaleax approved these changes Sep 24, 2016
View reviewed changes
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.

LGTM, CI: https://ci.nodejs.org/job/node-test-commit/5294/

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 24, 2016
jasnell pushed a commit that referenced this pull request Sep 26, 2016
@yosuke-furukawa @jasnell
util: Add format for SharedArrayBuffer
ce7d307 
PR-URL: #8587
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member

jasnell commented Sep 26, 2016

Landed in ce7d307. Thank you!

@jasnell jasnell closed this Sep 26, 2016
jasnell pushed a commit that referenced this pull request Sep 29, 2016
@yosuke-furukawa @jasnell
util: Add format for SharedArrayBuffer
e97fe46 
PR-URL: #8587
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
geek pushed a commit to geek/node that referenced this pull request Sep 30, 2016
@yosuke-furukawa @geek
util: Add format for SharedArrayBuffer
5eb6472 
PR-URL: nodejs#8587
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@yosuke-furukawa @Fishrock123
util: Add format for SharedArrayBuffer
fc68b12 
PR-URL: #8587
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alistairjcbrown alistairjcbrown alistairjcbrown left review comments

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell 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. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@yosuke-furukawa @Fishrock123 @addaleax @benjamingr @alistairjcbrown @jasnell @nodejs-github-bot