Land V8 4.6 onto master #3351
Land V8 4.6 onto master #3351
Conversation
|
LGTM, if CI is green |
|
I am a little concerned about how many commits we are back-porting from future V8 instead of just using the official stable version branches. I don't have any concrete feedback but I think it's something we should try to minimize in general. I am counting |
|
@domenic suggestion? Having slow and broken v8? |
|
Well, as I said I don't have any concrete feedback. But if these patches are not important enough for V8 to backport to 4.6 then the resulting V8 isn't going to be that slow and broken. Unless you are saying Chrome 46 is slow and broken. It's entirely possible that every single one of these |
|
@domenic oh certainly. I think we will be in a good shape when 4.7 will be stable, until now these patches are necessary for node to run fast. |
Update
Export post-mortem data for Node. Doubt they'd take the time to back port this.
Issue that caused our internal use of ArrayBuffers to crash on GC. Don't know if this is important enough for Chrome, but we can't function without it.
This one is a plain performance issue for v8. They should have back ported it by now.
Update to our tests. Nothing directly to do with v8.
Deals with |
|
LGTM but can you update the Reviewed-By tags in the backported patches? The backports themselves look correct to me. I echo @domenic's sentiment that the delta is pretty big right now. Remember that it's possible to CL cherry-picks with the tools/release/merge_to_branch.py script. I think the postmortem stuff, ArrayBuffer fixes and performance fixes are good candidates. SetAbortOnUncaughtExceptionCallback() probably isn't eligible because it's an API change. |
|
Also, is it necessary to have the |
ofrobots
force-pushed
the
vee-eight-4.6
branch
from
68c75ed
to
f2bb813
Compare
|
I agree with the sentiment about too many floating patches. I suggest the following two process changes:
@bnoordhuis I have dropped the
The |
|
+1 to @domenic's concerns with one major diversion: there are clearly situations where it's much more in Node's interest to expedite changes than it is for V8. The two that I'm seeing here are:
Beyond that, if the V8 team don't see it as urgent to back-port then I'd question us going out of the gate with it. Unfortunately we're going to be living with 4.6 for 6+ months so there's plenty of time to do backporting to fix stuff if it emerges over time that it really is important. |
|
We should not start with a presumption that V8 team is going to reject a merge-request for a Buffer or Post-mortem change (case in point: post-mortem change back-ported by the V8 team). We should at least request a merge back from upstream. Their disposition either way is helpful for us in deciding how to deal with the fix.
This is news to me. I thought semver-major changes can land on master, so I was expecting that 4.7 will land on master in ~6 weeks' time. Note that 4.7 isn't expected to have any actionable change to the API. |
That's correct but |
|
LGTM |
ofrobots
force-pushed
the
vee-eight-4.6
branch
from
f2bb813
to
71f1197
Compare
Included acb6779 4e028bd 75fe773 c22bcd3 49dec1a Not included (cannot be applied cleanly) 31450fc 2b8a06b PR-URL: nodejs#3351 Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
PR-URL: nodejs#3351 Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Some error messages have changed and the --debugger flag does not exist in V8 anymore. PR-URL: nodejs#3351 Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Backport 56a0a797f210e04746f2888116365d29a4bb6afc from V8 upstream to include post-mortem metadata used by mdb_v8 to support V8 4.6. Original commit message: Update post-mortem metadata generation mdb_v8, a post-mortem debugger for Node.js, now uses JSArrayBuffer's backing_store property and JSArrayBufferView's byte_offset property to get access to the content of Buffer instances in node (which are Uint8Array instances). This change adds post-mortem metadata for these two properties. This change also fixes a typo in inobject_properties_of_constructor_function_index_offset that was added to gen-postmortem-metadata in a previous change. It should be named inobject_properties_or_constructor_function_index instead. [email protected] Review URL: https://codereview.chromium.org/1363403003 Cr-Commit-Position: refs/heads/master@{nodejs#30926} Ref: nodejs#3057 PR-URL: nodejs#3351 Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
This a backport of the following commits from the v8's upstream:
* 1a8c38c50513f9af07ada479629a653e1cf36ff3
* 206f12abee3f1e7eda8fc6521d48f3c319460ee1
* 9e3676da9ab1aaf7de3e8582cb3fdefcc3dbaf33
Original commit message:
heap: make array buffer maps disjoint
Remove intersection from the `std::map`s representing current live
ArrayBuffers. While being simpler to understand, it poses
significant performance issue for the active ArrayBuffer users (like
node.js).
Store buffers separately, and process them together during
mark-sweep phase.
The results of benchmarks are:
$ ./node-slow bench && ./node-fast bench
4997.4 ns/op
4685.7 ns/op
NOTE: `fast` - was a patched node.js, `slow` - unpatched node.js
with vanilla v8.
Ref: nodejs#2732
PR-URL: nodejs#3351
Reviewed-By: indutny - Fedor Indutny <[email protected]>
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Original commit message:
[objects] do not visit ArrayBuffer's backing store
ArrayBuffer's backing store is a pointer to external heap, and
can't be treated as a heap object. Doing so will result in
crashes, when the backing store is unaligned.
See: nodejs#2791
BUG=chromium:530531
[email protected]
LOG=N
Review URL: https://codereview.chromium.org/1327403002
Cr-Commit-Position: refs/heads/master@{nodejs#30771}
Ref: nodejs#2791
Ref: nodejs#2912
PR-URL: nodejs#3351
Reviewed-By: indutny - Fedor Indutny <[email protected]>
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Move up to the latest patch level from the V8 4.6 branch: v8/v8@4.6.85.23...4.6.85.25 PR-URL: nodejs#3351 Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
ofrobots
force-pushed
the
vee-eight-4.6
branch
from
9f649f2
to
af58aea
Compare
On case-insensitive platforms, the Debug/ rule catches it. PR-URL: #3351 Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
PR-URL: #3351 Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Included acb6779 4e028bd 75fe773 c22bcd3 49dec1a Not included (cannot be applied cleanly) 31450fc 2b8a06b PR-URL: #3351 Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
PR-URL: #3351 Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Some error messages have changed and the --debugger flag does not exist in V8 anymore. PR-URL: #3351 Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Backport 56a0a797f210e04746f2888116365d29a4bb6afc from V8 upstream to include post-mortem metadata used by mdb_v8 to support V8 4.6. Original commit message: Update post-mortem metadata generation mdb_v8, a post-mortem debugger for Node.js, now uses JSArrayBuffer's backing_store property and JSArrayBufferView's byte_offset property to get access to the content of Buffer instances in node (which are Uint8Array instances). This change adds post-mortem metadata for these two properties. This change also fixes a typo in inobject_properties_of_constructor_function_index_offset that was added to gen-postmortem-metadata in a previous change. It should be named inobject_properties_or_constructor_function_index instead. [email protected] Review URL: https://codereview.chromium.org/1363403003 Cr-Commit-Position: refs/heads/master@{#30926} Ref: #3057 PR-URL: #3351 Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
This a backport of the following commits from the v8's upstream:
* 1a8c38c50513f9af07ada479629a653e1cf36ff3
* 206f12abee3f1e7eda8fc6521d48f3c319460ee1
* 9e3676da9ab1aaf7de3e8582cb3fdefcc3dbaf33
Original commit message:
heap: make array buffer maps disjoint
Remove intersection from the `std::map`s representing current live
ArrayBuffers. While being simpler to understand, it poses
significant performance issue for the active ArrayBuffer users (like
node.js).
Store buffers separately, and process them together during
mark-sweep phase.
The results of benchmarks are:
$ ./node-slow bench && ./node-fast bench
4997.4 ns/op
4685.7 ns/op
NOTE: `fast` - was a patched node.js, `slow` - unpatched node.js
with vanilla v8.
Ref: #2732
PR-URL: #3351
Reviewed-By: indutny - Fedor Indutny <[email protected]>
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Original commit message:
[objects] do not visit ArrayBuffer's backing store
ArrayBuffer's backing store is a pointer to external heap, and
can't be treated as a heap object. Doing so will result in
crashes, when the backing store is unaligned.
See: #2791
BUG=chromium:530531
[email protected]
LOG=N
Review URL: https://codereview.chromium.org/1327403002
Cr-Commit-Position: refs/heads/master@{#30771}
Ref: #2791
Ref: #2912
PR-URL: #3351
Reviewed-By: indutny - Fedor Indutny <[email protected]>
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Move up to the latest patch level from the V8 4.6 branch: v8/v8@4.6.85.23...4.6.85.25 PR-URL: #3351 Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
|
Thanks. Landed in Ping @misterdjules, in case you want to test post-mortem with V8 4.6. |
|
@ofrobots I had tested the vee-eight-4.6 branch regularly as changes were added. Tested again just now, it's all good, thank you for the heads up! |
Fishrock123
added
the
semver-major
|
@misterdjules is that sign-off for v5 with this version of V8 with postmortem? I was going to make it a checklist item for v5.0.0 but it sounds like you're giving it the 👍 |
|
@rvagg Interesting, I'm pretty confident that I had updated the check list to check "Prepare postmortem metadata in new V8 prior to 5.0.0" as indicated in one of my comments. I checked it again :) |
Notable changes: * buffer: (Breaking) Removed both 'raw' and 'raws' encoding types from Buffer, these have been deprecated for a long time (Sakthipriyan Vairamani) nodejs#2859. * console: (Breaking) Values reported by console.time() now have 3 decimals of accuracy added (Michaël Zasso) nodejs#3166. * fs: - fs.readFile*(), fs.writeFile*(), and fs.appendFile*() now also accept a file descriptor as their first argument (Johannes Wüller) nodejs#3163. - (Breaking) In fs.readFile(), if an encoding is specified and the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) nodejs#3485. - (Breaking) In fs.read() (using the fs.read(fd, length, position, encoding, callback) form), if the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) nodejs#3503. * http: - Fixed a bug where pipelined http requests would stall (Fedor Indutny) nodejs#3342. - (Breaking) When parsing HTTP, don't add duplicates of the following headers: Retry-After, ETag, Last-Modified, Server, Age, Expires. This is in addition to the following headers which already block duplicates: Content-Type, Content-Length, User-Agent, Referer, Host, Authorization, Proxy-Authorization, If-Modified-Since, If-Unmodified-Since, From, Location, Max-Forwards (James M Snell) nodejs#3090. - (Breaking) The callback argument to OutgoingMessage#setTimeout() must be a function or a TypeError is thrown (James M Snell) nodejs#3090. - (Breaking) HTTP methods and header names must now conform to the RFC 2616 "token" rule, a list of allowed characters that excludes control characters and a number of separator characters. Specifically, methods and header names must now match /^[a-zA-Z0-9_!#$%&'*+.^`|~-]+$/ or a TypeError will be thrown (James M Snell) nodejs#2526. * node: - (Breaking) Deprecated the _linklist module (Rich Trott) nodejs#3078. - (Breaking) Removed require.paths and require.registerExtension(), both had been previously set to throw Error when accessed (Sakthipriyan Vairamani) nodejs#2922. * npm: Upgraded to version 3.3.6 from 2.14.7, see https://github.com/npm/npm/releases/tag/v3.3.6 for more details. This is a major version bump for npm and it has seen a significant amount of change. Please see the original npm v3.0.0 release notes for a list of major changes (Rebecca Turner) nodejs#3310. * src: (Breaking) Bumped NODE_MODULE_VERSION to 47 from 46, this is necessary due to the V8 upgrade. Native add-ons will need to be recompiled (Rod Vagg) nodejs#3400. * timers: Attempt to reuse the timer handle for setTimeout().unref(). This fixes a long-standing known issue where unrefed timers would perviously hold beforeExit open (Fedor Indutny) nodejs#3407. * tls: - Added ALPN Support (Shigeki Ohtsu) nodejs#2564. - TLS options can now be passed in an object to createSecurePair() (Коренберг Марк) nodejs#2441. - (Breaking) The default minimum DH key size for tls.connect() is now 1024 bits and a warning is shown when DH key size is less than 2048 bits. This a security consideration to prevent "logjam" attacks. A new minDHSize TLS option can be used to override the default. (Shigeki Ohtsu) nodejs#1831. * util: - (Breaking) util.p() was deprecated for years, and has now been removed (Wyatt Preul) nodejs#3432. - (Breaking) util.inherits() can now work with ES6 classes. This is considered a breaking change because of potential subtle side-effects caused by a change from directly reassigning the prototype of the constructor using `ctor.prototype = Object.create(superCtor.prototype, { constructor: { ... } })` to using `Object.setPrototypeOf(ctor.prototype, superCtor.prototype)` (Michaël Zasso) nodejs#3455. * v8: (Breaking) Upgraded to 4.6.85.25 from 4.5.103.35 (Ali Ijaz Sheikh) nodejs#3351. - Implements the spread operator, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator for further information. - Implements new.target, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target for further information. * zlib: Decompression now throws on truncated input (e.g. unexpected end of file) (Yuval Brik) nodejs#2595. PR-URL: nodejs#3466
Notable changes: * buffer: (Breaking) Removed both 'raw' and 'raws' encoding types from Buffer, these have been deprecated for a long time (Sakthipriyan Vairamani) #2859. * console: (Breaking) Values reported by console.time() now have 3 decimals of accuracy added (Michaël Zasso) #3166. * fs: - fs.readFile*(), fs.writeFile*(), and fs.appendFile*() now also accept a file descriptor as their first argument (Johannes Wüller) #3163. - (Breaking) In fs.readFile(), if an encoding is specified and the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) #3485. - (Breaking) In fs.read() (using the fs.read(fd, length, position, encoding, callback) form), if the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) #3503. * http: - Fixed a bug where pipelined http requests would stall (Fedor Indutny) #3342. - (Breaking) When parsing HTTP, don't add duplicates of the following headers: Retry-After, ETag, Last-Modified, Server, Age, Expires. This is in addition to the following headers which already block duplicates: Content-Type, Content-Length, User-Agent, Referer, Host, Authorization, Proxy-Authorization, If-Modified-Since, If-Unmodified-Since, From, Location, Max-Forwards (James M Snell) #3090. - (Breaking) The callback argument to OutgoingMessage#setTimeout() must be a function or a TypeError is thrown (James M Snell) #3090. - (Breaking) HTTP methods and header names must now conform to the RFC 2616 "token" rule, a list of allowed characters that excludes control characters and a number of separator characters. Specifically, methods and header names must now match /^[a-zA-Z0-9_!#$%&'*+.^`|~-]+$/ or a TypeError will be thrown (James M Snell) #2526. * node: - (Breaking) Deprecated the _linklist module (Rich Trott) #3078. - (Breaking) Removed require.paths and require.registerExtension(), both had been previously set to throw Error when accessed (Sakthipriyan Vairamani) #2922. * npm: Upgraded to version 3.3.6 from 2.14.7, see https://github.com/npm/npm/releases/tag/v3.3.6 for more details. This is a major version bump for npm and it has seen a significant amount of change. Please see the original npm v3.0.0 release notes for a list of major changes (Rebecca Turner) #3310. * src: (Breaking) Bumped NODE_MODULE_VERSION to 47 from 46, this is necessary due to the V8 upgrade. Native add-ons will need to be recompiled (Rod Vagg) #3400. * timers: Attempt to reuse the timer handle for setTimeout().unref(). This fixes a long-standing known issue where unrefed timers would perviously hold beforeExit open (Fedor Indutny) #3407. * tls: - Added ALPN Support (Shigeki Ohtsu) #2564. - TLS options can now be passed in an object to createSecurePair() (Коренберг Марк) #2441. - (Breaking) The default minimum DH key size for tls.connect() is now 1024 bits and a warning is shown when DH key size is less than 2048 bits. This a security consideration to prevent "logjam" attacks. A new minDHSize TLS option can be used to override the default. (Shigeki Ohtsu) #1831. * util: - (Breaking) util.p() was deprecated for years, and has now been removed (Wyatt Preul) #3432. - (Breaking) util.inherits() can now work with ES6 classes. This is considered a breaking change because of potential subtle side-effects caused by a change from directly reassigning the prototype of the constructor using `ctor.prototype = Object.create(superCtor.prototype, { constructor: { ... } })` to using `Object.setPrototypeOf(ctor.prototype, superCtor.prototype)` (Michaël Zasso) #3455. * v8: (Breaking) Upgraded to 4.6.85.25 from 4.5.103.35 (Ali Ijaz Sheikh) #3351. - Implements the spread operator, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator for further information. - Implements new.target, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target for further information. * zlib: Decompression now throws on truncated input (e.g. unexpected end of file) (Yuval Brik) #2595. PR-URL: #3466
Now that V8 4.6 has hit stable, this PR brings-in the latest V8 4.6 patch level (4.6.85.25) after rebasing
vee-eight-4.6ontomasteras of this morning. ALL of the commits on this PR need to be reviewed as they were not reviewed when landing onvee-eight-4.6.Ref: #2688
R=@bnoordhuis,@nodejs/v8