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: forward compatibility to current V8 master #12875

Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented May 6, 2017

As discussed in nodejs/CTC#99, we are aiming for a upgrade to V8 6.0 in the Node 8.x lifecycle. Therefore, it makes sense to apply changes that are semver-major because they break the API or ABI before that upgrade actually happens.

Changes that are marked deps: are cherry-picks, the rest is manual work and needs review.

This is a bit WIP because I’m still compiling to see if everything works out. ;)

CI: https://ci.nodejs.org/job/node-test-commit/9702/
CI: https://ci.nodejs.org/job/node-test-commit/9705/
CI: https://ci.nodejs.org/job/node-test-commit/9720/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/683/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/685/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/686/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/687/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/691/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/763/
CITGM on master for comparison: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/764/

@nodejs/v8 @targos @jasnell

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
  • figure out whether to do anything about removed command line flags
Affected core subsystem(s)

deps/v8

mlippautz and others added 8 commits May 6, 2017 20:26
Original commit message:

    Remove object grouping

    Enbedders should switch to EmbedderHeapTracer API.

    BUG=v8:5828

    Change-Id: I82f2bc583d246617865a17f5904e02cd35f92fec
    Reviewed-on: https://chromium-review.googlesource.com/448539
    Reviewed-by: Hannes Payer <[email protected]>
    Reviewed-by: Ulan Degenbaev <[email protected]>
    Commit-Queue: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#43551}

Ref: https://chromium-review.googlesource.com/448539
Ref: v8/v8@78867ad8707a016
Original commit message:

    Drop UniqueId from include/v8.h

    It's unused since March 2 2017 (https://chromium-review.googlesource.com/448539).
    This removes it assuming that leaving it in the header was an oversight.

    BUG=v8:5828

    Review-Url: https://codereview.chromium.org/2732803002
    Cr-Commit-Position: refs/heads/master@{nodejs#43605}

Ref: v8/v8@de1461b7efd
Original commit message:

    Remove experimental fast accessor builder API

    As the code isn't used, but would have to be ported from hand-written
    assembly to CodeStubAssembler anyways, I propose to remove it and
    restore it if we decide that we actually need it.

    [email protected]
    BUG=

    Change-Id: Iffd7fc6ec534b1dd7a9144da900424355c8a7a02
    Reviewed-on: https://chromium-review.googlesource.com/453461
    Commit-Queue: Jochen Eisinger <[email protected]>
    Reviewed-by: Daniel Vogelheim <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#43763}

Ref: v8/v8@2cd2f5feff3
Original commit message:

    Make v8::Eternal::Get and IsEmpty const.

    They do not modify the state of the handle.

    Review-Url: https://codereview.chromium.org/2753973002
    Cr-Commit-Position: refs/heads/master@{nodejs#43907}

Ref: v8/v8@3700a01c82
Original commit message:

    Give v8::Eternal a direct reference to the handle.

    This makes it more similar to other handle types (like PersistentBase),
    by simply storing an i::Object** cast to T*. This means that it is not
    necessary to look up the handle in the eternal handles table to access
    the underlying value.

    Like the built-in roots (null, etc.), an eternal handle can never be
    destroyed, so we don't even need to allocate a separate local handle.
    Instead, the Local<T> can point directly at the eternal reference.
    This makes Eternal<T>::Get trivial.

    Review-Url: https://codereview.chromium.org/2751263003
    Cr-Commit-Position: refs/heads/master@{nodejs#43912}

Ref: v8/v8@4acdb5eec2c
Backport new virtual methods from
99743ad460e (“[wasm] Transferrable modules”).

Ref: https://codereview.chromium.org/2748473004
Ref: v8/v8@99743ad460e
Backport removed methods from 6226576efa82ee (“[wasm] Deleted old way
of checking embedder limits on wasm size.”).

Ref: https://codereview.chromium.org/2772203005
Ref: v8/v8@6226576efa82ee
Backport deprecation of `Context::EstimatedSize()` from
da5b745dba387 (“[api] deprecate unused context size estimate”).

Ref: v8/v8@da5b745
@addaleax addaleax added wip Issues and PRs that are still a work in progress. semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency. v8.x labels May 6, 2017
@addaleax addaleax added this to the 8.0.0 milestone May 6, 2017
@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label May 6, 2017
@addaleax
Copy link
Member Author

addaleax commented May 6, 2017

This works locally, so I kicked off CI.

Flags that would still be removed:

-  --datetime_format_to_parts (enable "Intl.DateTimeFormat.formatToParts")
-  --future (Implies all staged features that we want to ship in the not-too-far future)
-  --ignition_staging (use ignition with all staged features)
-  --ignition_deadcode (use ignition dead code elimination optimizer)
-        type: bool  default: true
-  --ignition_peephole (use ignition peephole optimizer)
-  --collect_megamorphic_maps_from_stub_cache (crankshaft harvests type feedback from stub cache)
-        type: bool  default: false
-  --trace_wasm_text_start (start function for WASM text generation (inclusive))
-        type: int  default: 0
-  --trace_wasm_text_end (end function for WASM text generation (exclusive))
-        type: int  default: 0
-  --wasm_loop_assignment_analysis (perform loop assignment analysis for WASM)
-        type: bool  default: true
-  --wasm_trap_if (enable the use of the trap_if operator for traps)
-        type: bool  default: true
-  --zap_code_space (Zap free memory in code space with 0xCC while sweeping.)
-        type: bool  default: false
-  --enable_fast_array_builtins (use optimized builtins)
-  --max_opt_count (maximum number of optimization attempts before giving up.)
-  --trace_debug_json (trace debugging JSON request/response)
-        type: bool  default: false
-  --trace_object_groups (print object groups detected during each garbage collection)
-        type: bool  default: false
-  --preparser_scope_analysis (perform scope analysis for preparsed inner functions)

Remaining diff to current V8 master include/v8.h: https://gist.github.com/addaleax/4cb6d8dc35e4c4adfe89b8d55680f0f6

@addaleax addaleax removed the wip Issues and PRs that are still a work in progress. label May 6, 2017
@addaleax addaleax force-pushed the v8-5.8-6.0-forward-compat branch from 0727030 to 41f6222 Compare May 6, 2017 19:46
addaleax and others added 5 commits May 6, 2017 22:25
Backport ABI-incompatible changes from
bf463c4dc080 (“[async-iteration] implement AsyncGenerator”) and
dc662e5b740c (“[inspector] move console to builtins”).

This also requires relaxing one of the V8 unit tests.

Ref: https://chromium-review.googlesource.com/446961
Ref: v8/v8@bf463c4dc080
Ref: https://codereview.chromium.org/2785293002
Ref: v8/v8@dc662e5b740c

[squash] fixup V8 test
Backport ABI-incompatible changes from
94283dcf4459f (“[ESNext] Implement DynamicImportCall”).

Ref: https://codereview.chromium.org/2703563002
Ref: v8/v8@94283dcf4459f
Backport API-incompatible changes from
2e4a68733803 (“[v8] v8::StackTrace::AsArray returns correct array”).

Ref: https://codereview.chromium.org/2806373005
Ref: https://github.com/v8/v8/2e4a68733803
Original commit message:

    Add documentation for FunctionCallbackInfo

    [email protected],[email protected],[email protected]
    BUG=

    Change-Id: I273f5ce305f80b2aa5e9c8c42a6e8e5afc51a0a7
    Reviewed-on: https://chromium-review.googlesource.com/484422
    Reviewed-by: Kentaro Hara <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Jochen Eisinger <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#44927}

Ref: v8/v8@4fdf9fd4813
@addaleax addaleax force-pushed the v8-5.8-6.0-forward-compat branch from 41f6222 to 932bd46 Compare May 6, 2017 20:25
@targos
Copy link
Member

targos commented May 6, 2017

I suppose we can have this only on v8.x? That way we will be able to simply update master to 5.9 when it's stable.
Also is it compatible with this fix from the V8 team?: v8@eb87979

@addaleax
Copy link
Member Author

addaleax commented May 6, 2017

I suppose we can have this only on v8.x? That way we will be able to simply update master to 5.9 when it's stable.

If you prefer that, yes, I guess so. Edit: I’ll just switch the base branch to v8.x-staging once V8 5.8 is on it.

Also is it compatible with this fix from the V8 team?: v8/node@eb87979

Heh, well, from it quick glance it looks that way – it’s a bit hard to tell since everything is squashed into one big change. Edit: See #12878 (comment) for a better answer :)

Backport ABI-incompatible changes from
dab18fb0bbcdd (“Make idle tasks optional in the default platform.”).

Ref: https://codereview.chromium.org/2737743002
Ref: v8/v8@dab18fb0bbcdd
@addaleax
Copy link
Member Author

addaleax commented May 7, 2017

Updated with one extra change from #12878 that seems to make sense (though it probably doesn’t actually affect addons anyway). The diff between the two PRs looks pretty nice to me, which seems like a good sign this will work out. :)

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.

Rubber stamp LGTM from me. I haven't gone through the code in detail, but I applied the patch and gave it some tests and everything appears to work as expected.

jasnell pushed a commit that referenced this pull request May 28, 2017
Backport ABI-incompatible changes from
dab18fb0bbcdd (“Make idle tasks optional in the default platform.”).

Ref: https://codereview.chromium.org/2737743002
Ref: v8/v8@dab18fb0bbcdd

PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Jun 7, 2017
2 tasks
addaleax pushed a commit that referenced this pull request Jun 17, 2017
Original commit message:

    Add documentation for FunctionCallbackInfo

    [email protected],[email protected],[email protected]
    BUG=

    Change-Id: I273f5ce305f80b2aa5e9c8c42a6e8e5afc51a0a7
    Reviewed-on: https://chromium-review.googlesource.com/484422
    Reviewed-by: Kentaro Hara <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Jochen Eisinger <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#44927}

Ref: v8/v8@4fdf9fd4813

PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Jun 17, 2017
PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax mentioned this pull request Jun 17, 2017
addaleax pushed a commit that referenced this pull request Jun 21, 2017
Original commit message:

    Add documentation for FunctionCallbackInfo

    [email protected],[email protected],[email protected]
    BUG=

    Change-Id: I273f5ce305f80b2aa5e9c8c42a6e8e5afc51a0a7
    Reviewed-on: https://chromium-review.googlesource.com/484422
    Reviewed-by: Kentaro Hara <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Jochen Eisinger <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#44927}

Ref: v8/v8@4fdf9fd4813

PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Jun 21, 2017
PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jun 24, 2017
Original commit message:

    Add documentation for FunctionCallbackInfo

    [email protected],[email protected],[email protected]
    BUG=

    Change-Id: I273f5ce305f80b2aa5e9c8c42a6e8e5afc51a0a7
    Reviewed-on: https://chromium-review.googlesource.com/484422
    Reviewed-by: Kentaro Hara <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Jochen Eisinger <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#44927}

Ref: v8/v8@4fdf9fd4813

PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Jun 24, 2017
PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 29, 2017
Original commit message:

    Add documentation for FunctionCallbackInfo

    [email protected],[email protected],[email protected]
    BUG=

    Change-Id: I273f5ce305f80b2aa5e9c8c42a6e8e5afc51a0a7
    Reviewed-on: https://chromium-review.googlesource.com/484422
    Reviewed-by: Kentaro Hara <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Jochen Eisinger <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#44927}

Ref: v8/v8@4fdf9fd4813

PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 29, 2017
PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
Original commit message:

    Add documentation for FunctionCallbackInfo

    [email protected],[email protected],[email protected]
    BUG=

    Change-Id: I273f5ce305f80b2aa5e9c8c42a6e8e5afc51a0a7
    Reviewed-on: https://chromium-review.googlesource.com/484422
    Reviewed-by: Kentaro Hara <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Jochen Eisinger <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#44927}

Ref: v8/v8@4fdf9fd4813

PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Jul 11, 2017
PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
Original commit message:

    Add documentation for FunctionCallbackInfo

    [email protected],[email protected],[email protected]
    BUG=

    Change-Id: I273f5ce305f80b2aa5e9c8c42a6e8e5afc51a0a7
    Reviewed-on: https://chromium-review.googlesource.com/484422
    Reviewed-by: Kentaro Hara <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Jochen Eisinger <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#44927}

Ref: v8/v8@4fdf9fd4813

PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Jul 18, 2017
PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jul 21, 2017
Original commit message:

    Add documentation for FunctionCallbackInfo

    [email protected],[email protected],[email protected]
    BUG=

    Change-Id: I273f5ce305f80b2aa5e9c8c42a6e8e5afc51a0a7
    Reviewed-on: https://chromium-review.googlesource.com/484422
    Reviewed-by: Kentaro Hara <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Jochen Eisinger <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#44927}

Ref: v8/v8@4fdf9fd4813

PR-URL: nodejs#12875
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jul 21, 2017
addaleax pushed a commit that referenced this pull request Jul 24, 2017
Original commit message:

    Add documentation for FunctionCallbackInfo

    [email protected],[email protected],[email protected]
    BUG=

    Change-Id: I273f5ce305f80b2aa5e9c8c42a6e8e5afc51a0a7
    Reviewed-on: https://chromium-review.googlesource.com/484422
    Reviewed-by: Kentaro Hara <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Jochen Eisinger <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#44927}

Ref: v8/v8@4fdf9fd4813

PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Jul 24, 2017
PR-URL: #12875
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax mentioned this pull request Jul 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants