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.

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

v8.3.0 proposal #14452

Closed
wants to merge 64 commits into from
Closed

v8.3.0 proposal #14452

wants to merge 64 commits into from

Conversation

addaleax
Copy link
Member

2017-07-??, Version 8.3.0 (Current), @addaleax

Notable changes

  • V8
    • The V8 engine has been upgraded to version 5.9, which has a significantly
      changed performance profile.
      #13515

Commits


This does include V8 5.9 again.

/cc @evanlucas @MylesBorins @nodejs/ctc

BridgeAR and others added 30 commits July 24, 2017 10:58
Backport-PR-URL: #14392
Backport-Reviewed-By: James M Snell <[email protected]>

PR-URL: #13733
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
The way it is currently written, test-http-full-response will fail if
there is a problem with spawning that doesn't include `ab` or `api` in
`stderr`, but it will fail with a misleading mismatched-calls
`common.mustCall()` error.

Alter the error handling so that it rethrows the actual error, providing
better information.

PR-URL: #14252
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Adds a missing `'` in code example.

PR-URL: #14353
Fixes: #14352
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Use template literals instead of string concatenation in
test/parallel/test-http-extra-response.js

PR-URL: #14296
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: David Cai <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Add a note to the stream docs specifying that at most a single
call to _transform can happen, and the provided callback()
should be used to process another chunk.

Fixes: #3208
PR-URL: #14321
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Previous behavior was to assume an error is a proper error in the
repl module. A check was added to not terminate the process on thrown
repl errors that are `null` or `undefined`.

PR-URL: #14306
Fixes: #12373
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]
PR-URL: #14372
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #14287
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
fs.rmdir() on the file (not directory) results in different errors on
Windows to everything else

Fixes: #8797
PR-URL: #14323
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
PR-URL: #14342
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Allow passing the prefix in via the PKGDIR env var. This will allow us
to use this same script to codesign the binary tarball.

PR-URL: #14179
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: #14179
Fixes: #11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: #14272
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
PR-URL: #14280
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
test-fs-largefile was disabled. It was fixed in bbf74fb but left in
disabled because it generates a 5Gb file. However, gibfahn had the
sensible suggestion of moving it to the pummel directory. Which is what
this change does.

In pummel, lint rules are applied, so this does necessitate changing a
pair of `var` declarations to `const`.

PR-URL: #14338
Refs: bbf74fb
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: #14388
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Clarifies the default shell in Windows is process.env.ComSpec
and that cmd.exe is only used as a fallback. Functions whose
descriptions are affected include:
child_process.spawn, child_process.exec,
child_process.spawnsSync, and child_process.execSync.

PR-URL: #14203
Fixes: #14156
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Added comments to whatwg-url tests that they should not be changed until
modifications are merged upstream as per "Web Platform Tests" guidelines

PR-URL: #14355
Fixes: #12793
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: #14364
Fixes: #14362
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
PR-URL: #14319
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: #14286
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Line 486 and 525 contain for loops where a length property is cached in
a variable (for example, itemLen). This used to be a performance
optimization, but current V8 handles the optimization internally.

These caching variables are removed, and the length property is used
directly instead.

PR-URL: #14275
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
By adding a test case using a path with illegal protocol

PR-URL: #14301
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
This test was disabled in 2011 and is no longer useful without
modifications.

PR-URL: #14334
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Replace 250ms timer with event-based logic to make test robust.

PR-URL: #14361
Fixes: #13597
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: #14375
Refs: #13937
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Move test-http-server-keep-alive-timeout-slow-server and
test-http-server-keep-alive-timeout-slow-client-headers from parallel to
sequential to resolve test flakiness on freebsd10-64.

Fixes: #14033
Refs: #9317
PR-URL: #14377
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Also used in common.gypi to check whether a flag is needed or not
based on llvm version.

PR-URL: #14077
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@addaleax
Copy link
Member Author

isheludko and others added 2 commits July 24, 2017 18:40
Original commit message:

    Properly handle loads from global interceptor via prototype chain.

    ... when receiver is in dictionary mode.

    Bug: v8:6490
    Change-Id: Ic5a8d214adcc4efd4cb163cbc6b351c4e6b596af
    Reviewed-on: https://chromium-review.googlesource.com/559548
    Reviewed-by: Camillo Bruni <[email protected]>
    Commit-Queue: Igor Sheludko <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#46428}

Ref: https://chromium.googlesource.com/v8/v8.git/+/6cb999b97b7953ebfd4aabf2e1f62bf405f21c69
Fixes: #13804
PR-URL: #14188
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Notable changes

* **V8**
    * The V8 engine has been upgraded to version 5.9, which has a significantly
    changed performance profile.
    [#13515](#13515)
@evanlucas
Copy link
Contributor

I'm +1 on including V8 5.9 in this release. I think we should get TF+I into users hands sooner rather than later.

@evanlucas
Copy link
Contributor

@nodejs/release @nodejs/ctc anyone opposed to me cutting a RC for this one?

@mscdex
Copy link
Contributor

mscdex commented Jul 24, 2017

V8 6.0 goes stable for desktop in 2 days according to the Chromium calendar. For non-desktop(?) it goes stable in about a week. According to @bmeurer, 6.0 fixes a number of TurboFan-related performance issues. It might be better to just wait a bit longer (or maybe we can just ship 6.0 sooner if nothing is expected to change in 6.0 in a week) because of that, otherwise we could have more issues being submitted about performance degradation that are fixed in 6.0.

@bricss
Copy link

bricss commented Jul 24, 2017

@mscdex if you will take a look at here, you wouldn't see any kind of degradation. V8 6.0 will land in master very soon and it's better to make progressive releases before that moment.

@mscdex
Copy link
Contributor

mscdex commented Jul 24, 2017

@bricss I don't know what you're referring to in that thread, but @winksaville demonstrated a measurable difference between 5.9 and 6.1. I would bet at least some of that is due to the fixes made in 6.0.

As I said, waiting at most a week to ensure fixes the V8 team have landed in 6.0 is worth it. That's also the opinion of some of those on the V8 team.

@bricss
Copy link

bricss commented Jul 24, 2017

The takeaway is V8 6.1 is fastest followed by V8 5.9 and V8 5.8 is slowest:

**** Summary ES5 code:
nodev8-V8-5.8 20.07s
nodev9-V8-5.9 12.30s
nodev9-V8-6.1 12.13s

**** Summary ES6 code:
nodev8-V8-5.8 131.06s
nodev9-V8-5.9 19.36s
nodev9-V8-6.1 12.30s

Less is better? It seems that V8 5.9 is much faster than V8 5.8.

@mscdex
Copy link
Contributor

mscdex commented Jul 24, 2017

@bricss Right, and 6.1 is even faster. Besides, this is only one particular data point. There are threads elsewhere that show degradations in our own benchmarks with 5.9. Just recently even I found some performance issues in V8 5.9 that were solved in 6.x. So I still say we should skip 5.9 and go straight to 6.0 for the next update after 5.8.

@addaleax addaleax removed build Issues and PRs related to build files or the CI. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. v8 engine Issues and PRs related to the V8 dependency. doc Issues and PRs related to the documentations. labels Jul 24, 2017
@refack
Copy link
Contributor

refack commented Jul 24, 2017

If this is going to be the next release we need #14419 in (I'll land it as soon as CI finished). Landed.

@evanlucas
Copy link
Contributor

If we are only waiting a week, then I'm fine with waiting until 6.0, but I really think we need to get TF+I into our users hands ASAP.

@Fishrock123
Copy link
Contributor

Same tbh

@refack
Copy link
Contributor

refack commented Jul 25, 2017

AFAICT for node, V8 6.0 is still blocked on two issues, according to #14004
/cc @targos

@Trott
Copy link
Member

Trott commented Jul 25, 2017

Taking into account @mscdex's objections and looking at the emoji-voting in nodejs/CTC#155, it seems like there are currently more CTC members explicitly in favor of waiting for 6.0 than releasing with 5.9.

Of course, it's not a vote (yet, anyway). It's about consensus. And presumably none of the people who want 5.9 this week would be opposed to 6.0 next week. So I think that's where this is headed.

@winksaville
Copy link

If desired I'd be glad to test V8 6.0 with the same code on the same machine as mentioned above and originally posted here, if you can point me to a Node binary for linux-x64 with V8 6.0.

@evanlucas
Copy link
Contributor

Thanks @Trott. I had read through that issue last week and completely forgotten about it.

@MylesBorins
Copy link
Contributor

I'm -1 on including 5.9. We only have one more thing to fix in 6.0 to be able to land it, I'm hopefuly we can get to that today.

Will sign off when 5.9 commits have been dropped

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

Should not include 5.9 imho

@mcollina
Copy link
Member

I think we should wait for 6.0 to land, include that, and release 8.3.0 next week. I'm -1 on releasing a minor this week and one the week after.

@addaleax addaleax closed this Jul 27, 2017
@addaleax addaleax deleted the v8.3.0-proposal branch July 27, 2017 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.