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

lib: remove simd support from util.format() #11346

Closed
wants to merge 2 commits into from
Closed

lib: remove simd support from util.format() #11346

wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
Member

Upstream V8 is removing SIMD support. Be proactive and follow suit.

Refs: https://bugs.chromium.org/p/v8/issues/detail?id=4124

I figured that since it's going away anyway, we may save ourselves some maintenance work by removing it now rather than waiting until the V8 release that drops SIMD support. (And also because I'll probably forget if I don't act on it immediately.)

If people feel it's premature, I'm happy to postpone. The first commit does some minor cleanup and can land either way.

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Feb 13, 2017
jasnell
jasnell approved these changes Feb 13, 2017
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.

LGTM but I noticed that the v8 commit appears to have been reverted soon after landing.

@jasnell
Copy link
Member

jasnell commented Feb 13, 2017

should this actually go through a deprecation cycle tho? Is this still considered experimental?

@mscdex
Copy link
Contributor

mscdex commented Feb 13, 2017

Shouldn't the second commit message use util instead of lib for the subsystem?

@bnoordhuis
Copy link
Member Author

I noticed that the v8 commit appears to have been reverted soon after landing.

Because of CI failures. Temporary respite.

Is this still considered experimental?

It's never been non-experimental. :-)

Shouldn't the second commit message use util instead of lib for the subsystem?

I can update that.

@bnoordhuis
Copy link
Member Author

cc @fhinkel - I saw you reverted the V8 CL for breaking the integration build.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 13, 2017

@jasnell This probably doesn't need a deprecation cycle because it never worked without --harmony_simd on any released version afaik. I would still call this semver-major, though.

@fhinkel
Copy link
Member

fhinkel commented Feb 14, 2017 •
edited
Loading

@bnoordhuis yes, I wanted to keep the build green. I deleted the test in V8's Node fork and relanded the CL this morning, i.e., no simd.js in V8 master anymore!

I think @targos usually picks up the changes on V8's node fork when updating to a newer V8 version. But I guess removing support from master before current V8 lands doesn't hurt either.

+1 on removing an experimental feature without deprecation cycle.

We continuously integrate V8 master in Node. If anybody's interested: CI, V8's fork of Node.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 14, 2017 •
edited
Loading

@nodejs/ctc — do we consider changes or removal of flagged features as a semver-major or not?

@ChALkeR ChALkeR mentioned this pull request Feb 14, 2017
Closed
ofrobots
ofrobots approved these changes Feb 14, 2017
View reviewed changes
Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

LGTM. Agree on calling this a semver-major even though this was experimental.

@jasnell
Copy link
Member

jasnell commented Feb 14, 2017

I'm good with semver-major to be on the safe side

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 14, 2017
@TimothyGu
Copy link
Member

Not entirely related, but what is the reason for dropping SIMD support in V8?

@bnoordhuis
Copy link
Member Author

IANAV8TM (I Am Not A V8 Team Member) but it looks like it was partially maintenance effort, partially binary size. Someone reported that the Chrome .apk shrunk by almost 500 kB after removing SIMD.js.

jasnell pushed a commit that referenced this pull request Feb 15, 2017
@bnoordhuis @jasnell
test: drop v5.x-specific code path from simd test
c5d1851 
PR-URL: #11346
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this pull request Feb 15, 2017
@bnoordhuis @jasnell
lib: remove simd support from util.format()
2ba4eea 
Upstream V8 is removing SIMD support.  Be proactive and follow suit.

Refs: https://bugs.chromium.org/p/v8/issues/detail?id=4124
PR-URL: #11346
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member

jasnell commented Feb 15, 2017

Landed in c5d1851...2ba4eea

@jasnell jasnell closed this Feb 15, 2017
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
@screeny05
Copy link

For anyone further interested: https://bugs.chromium.org/p/v8/issues/detail?id=6020&desc=2

Most SIMD.js code we were able to find with performance wins is a variant on asm.js code, which requires carefully structured JS code, and which almost entirely comes from similar compilers to the sort used to generate wasm code. [...]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@evanlucas evanlucas evanlucas approved these changes

@jasnell jasnell jasnell approved these changes

@ChALkeR ChALkeR ChALkeR approved these changes

@ofrobots ofrobots ofrobots approved these changes

Assignees
No one assigned
Labels
semver-major PRs that contain breaking changes and should be released in the next major 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.