Skip to content

util: Add format for SharedArrayBuffer #8587

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

Fishrock123 commented Sep 17, 2016

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 Outdated
Copy link
Member

@addaleax addaleax Sep 17, 2016

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 Outdated
Copy link
Member

@addaleax addaleax Sep 17, 2016

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 Outdated
Copy link
Member

@addaleax addaleax Sep 17, 2016

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

addaleax commented Sep 17, 2016

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

yosuke-furukawa commented Sep 18, 2016

Thank you for your review, I fixed them.

@benjamingr
Copy link
Member

benjamingr commented Sep 18, 2016

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 Outdated
Copy link

@alistairjcbrown alistairjcbrown Sep 19, 2016

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

@yosuke-furukawa yosuke-furukawa Sep 19, 2016

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 Outdated
Copy link

@alistairjcbrown alistairjcbrown Sep 19, 2016

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

@yosuke-furukawa yosuke-furukawa Sep 19, 2016

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 Outdated
Copy link

@alistairjcbrown alistairjcbrown Sep 19, 2016

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

@yosuke-furukawa yosuke-furukawa Sep 19, 2016

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

yosuke-furukawa commented Sep 19, 2016

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

@benjamingr
Copy link
Member

benjamingr commented Sep 19, 2016

@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

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

+1 more reviewer

@alistairjcbrown alistairjcbrown alistairjcbrown left review comments

Reviewers whose approvals may not affect merge requirements

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