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

deps: backport rehash strings after deserialization #14345

Closed
wants to merge 1 commit into from
Closed

deps: backport rehash strings after deserialization #14345

wants to merge 1 commit into from

Conversation

hashseed
Copy link
Member

@hashseed hashseed commented Jul 18, 2017 •
edited

This consists of back merges of upstream changes, fixes due to
merge conflicts, and minor changes to V8's bootstrapper when
setting up built-in objects.

Fixes: #14171
Refs: https://goo.gl/6aN8xA
Refs: https://crrev.com/a2ab1353f6708b44d305fdd9fe65a6d29b95c6d6
Refs: https://crrev.com/182caaf4a9b94024e47007d426831c024345cb97

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

v8

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jul 18, 2017
@hashseed
Copy link
Member Author

@fhinkel @MylesBorins @ofrobots

@MylesBorins
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/9221/
V8-Ci: https://ci.nodejs.org/job/node-test-commit-v8-linux/793/

@MylesBorins
Copy link
Member

It is worth noting that if this lands before #14004 it will need to be manually included in that PR

@MylesBorins
Copy link
Member

One more thing (sorry about spam). If the first pass of CI is green we will likely want to include a revert of eff636d so we can properly tests with snapshots re-enabled

@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Closed
8 tasks
@ofrobots
Copy link
Contributor

Thanks for putting this together. Some nits: The V8 version should also be bumped as part of the backport. I would suggest formatting the commit message as per this guide. Also, github commit urls are preferrable to crrev.com.

Something like:

deps: backport rehash strings after deserialization

Original commit messages:
https://github.com/v8/v8/commit/a2ab1353f6708b44d305fdd9fe65a6d29b95c6d6
  [snapshot] Rehash strings after deserialization.

  See https://goo.gl/6aN8xA

  Bug: v8:6593
  Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
  Change-Id: Ic8b0b57195d01d41591397d5d45de3f0f3ebc3d9
  Reviewed-on: https://chromium-review.googlesource.com/574527
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46732}

https://github.com/v8/v8/182caaf4a9b94024e47007d426831c024345cb97
 Do not track transitions for built-in objects.

 Objects created during bootstrapping do not need a transition
 tree except for elements kind transitions.

 R=ishell@chromium.org

 Bug: v8:6596
 Change-Id: I237b8b2792f201336e1c9731c815095dd06bc182
 Reviewed-on: https://chromium-review.googlesource.com/571750
 Reviewed-by: Igor Sheludko <ishell@chromium.org>
 Commit-Queue: Yang Guo <yangguo@chromium.org>
 Cr-Commit-Position: refs/heads/master@{#46693}

Fixes: https://github.com/nodejs/node/issues/14171
PR-URL: https://github.com/nodejs/node/pull/14345

@hashseed
Copy link
Member Author

hashseed commented Jul 18, 2017 •
edited

Updated PR:

  • new commit message as suggested
  • re-enabled snapshot by default
  • bumped V8 version
  • rebaselined test expectation

@MylesBorins
Copy link
Member

@hashseed can you give this a rebase against master, it appears a wild merge commit has appeared.

@hashseed
Copy link
Member Author

hashseed commented Jul 19, 2017 •
edited

I just rebased onto b61cab2.

@hashseed
Copy link
Member Author

@MylesBorins could you trigger another CI run? This time with the snapshot enabled.

@ofrobots
Copy link
Contributor

New CI: https://ci.nodejs.org/job/node-test-pull-request/9263/

@targos
Copy link
Member

targos commented Jul 20, 2017

V8 CI for the hash seed test:https://ci.nodejs.org/job/node-test-commit-v8-linux/799/

@hashseed
Copy link
Member Author

FWIW the upstream changes do not seem to cause any issues in Chrome Canary.

Should I squash the commits so that there is only one commit, with the correctly formatted commit message?

@hashseed hashseed changed the title v8: rehash strings after deserialization deps: backport rehash strings after deserialization. Jul 20, 2017
@hashseed
deps: backport rehash strings after deserialization
404ade3 
Original commit messages:
v8/v8@a2ab135
  [snapshot] Rehash strings after deserialization.

  See https://goo.gl/6aN8xA

  Bug: v8:6593
  Change-Id: Ic8b0b57195d01d41591397d5d45de3f0f3ebc3d9
  Reviewed-on: https://chromium-review.googlesource.com/574527
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#46732}

v8/v8@182caaf
  Do not track transitions for built-in objects.

  Objects created during bootstrapping do not need
  a transition tree except for elements kind transitions.

  Bug: v8:6596
  Change-Id: I237b8b2792f201336e1c9731c815095dd06bc182
  Reviewed-on: https://chromium-review.googlesource.com/571750
  Reviewed-by: Igor Sheludko <ishell@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#46693}

Fixes: nodejs#14171
PR-URL: nodejs#14345
@hashseed hashseed changed the title deps: backport rehash strings after deserialization. deps: backport rehash strings after deserialization Jul 20, 2017
@hashseed
Copy link
Member Author

I squashed the commits together and fixed up the commit message a bit more.

@hashseed hashseed mentioned this pull request Jul 20, 2017
Closed
refack
refack approved these changes Jul 20, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Rubberstamp LGTM
configure changes LGTM
Assuming test-v8 and test/pummel/test-hash-seed.js are green

configure
@@ -960,7 +960,7 @@ def configure_v8(o):
o['variables']['v8_optimized_debug'] = 0 # Compile with -O0 in debug builds.
o['variables']['v8_random_seed'] = 0 # Use a random seed for hash tables.
o['variables']['v8_promise_internal_field_count'] = 1 # Add internal field to promises for async hooks.
o['variables']['v8_use_snapshot'] = b(options.with_snapshot)
o['variables']['v8_use_snapshot'] = 'false' if options.without_snapshot else 'true'
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: use b(want_snapshots)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a revert of the commit that disabled snapshots so I'd rather not change that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack.

bnoordhuis
bnoordhuis approved these changes Jul 20, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

deps/v8/src/snapshot/deserializer.cc
@@ -151,6 +153,9 @@ MaybeHandle<Object> Deserializer::DeserializePartial(
// changed and logging should be added to notify the profiler et al of the
// new code, which also has to be flushed from instruction cache.
CHECK_EQ(start_address, code_space->top());

if (FLAG_rehash_snapshot && can_rehash_) RehashContext(Context::cast(root));
Copy link
Member

Choose a reason for hiding this comment

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

For my own curiosity, root is logically always a Context here? I followed the logic down to Deserializer::ReadData() but things got complex fast from there on.

@hashseed
Copy link
Member Author

In partial-serializer.cc we only set can_be_rehashed_ to true if we are serializing a native context, so...

@bnoordhuis
Copy link
Member

Ah, okay. I wondered if it was an intrinsic property of the VisitPointer() call a few lines up.

@hashseed
Copy link
Member Author

It doesn't really matter in practice. We really only use this to serialize contexts. Only exceptions are some ancient test cases.

@addaleax
Copy link
Member

Fresh CIs because the old links give 404s:

CI: https://ci.nodejs.org/job/node-test-commit/11419/
V8 CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/817/

I assume this is ready to be landed, so I’d like to merge this once CI comes back good.

@addaleax
Copy link
Member

Landed in 8dce05f 🎉

@addaleax addaleax closed this Jul 28, 2017
addaleax pushed a commit that referenced this pull request Jul 28, 2017
@hashseed @addaleax
deps: backport rehash strings after deserialization
8dce05f 
Original commit messages:
v8/v8@a2ab135
  [snapshot] Rehash strings after deserialization.

  See https://goo.gl/6aN8xA

  Bug: v8:6593
  Change-Id: Ic8b0b57195d01d41591397d5d45de3f0f3ebc3d9
  Reviewed-on: https://chromium-review.googlesource.com/574527
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46732}

v8/v8@182caaf
  Do not track transitions for built-in objects.

  Objects created during bootstrapping do not need
  a transition tree except for elements kind transitions.

  Bug: v8:6596
  Change-Id: I237b8b2792f201336e1c9731c815095dd06bc182
  Reviewed-on: https://chromium-review.googlesource.com/571750
  Reviewed-by: Igor Sheludko <ishell@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46693}

Fixes: #14171
PR-URL: #14345
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@addaleax
Copy link
Member

I’m not sure whether that’s obvious or not, but: This doesn’t apply cleanly on v8.x-staging (V8 5.8). I guess that means we’re waiting for V8 6.0?

@hashseed
Copy link
Member Author

I can create a PR against 5.8 of that still makes sense. I'll also check whether it applies to 6.0 if someone can point me to a branch where I can do that.

@addaleax
Copy link
Member

@hashseed The PR for V8 6.0 is @ #14004, you can try to do things on top of that :) I wouldn’t worry about 5.8 for now.

@MylesBorins
Copy link
Member

MylesBorins commented Jul 28, 2017 •
edited

6.0 backport included as 2ae2874

@hashseed
Copy link
Member Author

Thanks Myles!

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Aug 1, 2017
@hashseed @MylesBorins
deps: backport rehash strings after deserialization
424aed0 
Original commit messages:
v8/v8@a2ab135
  [snapshot] Rehash strings after deserialization.

  See https://goo.gl/6aN8xA

  Bug: v8:6593
  Change-Id: Ic8b0b57195d01d41591397d5d45de3f0f3ebc3d9
  Reviewed-on: https://chromium-review.googlesource.com/574527
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#46732}

v8/v8@182caaf
  Do not track transitions for built-in objects.

  Objects created during bootstrapping do not need
  a transition tree except for elements kind transitions.

  Bug: v8:6596
  Change-Id: I237b8b2792f201336e1c9731c815095dd06bc182
  Reviewed-on: https://chromium-review.googlesource.com/571750
  Reviewed-by: Igor Sheludko <ishell@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#46693}

Fixes: nodejs#14171
PR-URL: nodejs#14345
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Aug 1, 2017
@hashseed @MylesBorins
deps: backport rehash strings after deserialization
5d5e31c 
Original commit messages:
v8/v8@a2ab135
  [snapshot] Rehash strings after deserialization.

  See https://goo.gl/6aN8xA

  Bug: v8:6593
  Change-Id: Ic8b0b57195d01d41591397d5d45de3f0f3ebc3d9
  Reviewed-on: https://chromium-review.googlesource.com/574527
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#46732}

v8/v8@182caaf
  Do not track transitions for built-in objects.

  Objects created during bootstrapping do not need
  a transition tree except for elements kind transitions.

  Bug: v8:6596
  Change-Id: I237b8b2792f201336e1c9731c815095dd06bc182
  Reviewed-on: https://chromium-review.googlesource.com/571750
  Reviewed-by: Igor Sheludko <ishell@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#46693}

Fixes: nodejs#14171
Refs: nodejs#14345

PR-URL: nodejs#14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 1, 2017
@hashseed @MylesBorins
deps: backport rehash strings after deserialization
98ddab4 
Original commit messages:
v8/v8@a2ab135
  [snapshot] Rehash strings after deserialization.

  See https://goo.gl/6aN8xA

  Bug: v8:6593
  Change-Id: Ic8b0b57195d01d41591397d5d45de3f0f3ebc3d9
  Reviewed-on: https://chromium-review.googlesource.com/574527
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46732}

v8/v8@182caaf
  Do not track transitions for built-in objects.

  Objects created during bootstrapping do not need
  a transition tree except for elements kind transitions.

  Bug: v8:6596
  Change-Id: I237b8b2792f201336e1c9731c815095dd06bc182
  Reviewed-on: https://chromium-review.googlesource.com/571750
  Reviewed-by: Igor Sheludko <ishell@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46693}

Fixes: #14171
Refs: #14345

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 2, 2017
@hashseed @addaleax
deps: backport rehash strings after deserialization
e2b306c 
Original commit messages:
v8/v8@a2ab135
  [snapshot] Rehash strings after deserialization.

  See https://goo.gl/6aN8xA

  Bug: v8:6593
  Change-Id: Ic8b0b57195d01d41591397d5d45de3f0f3ebc3d9
  Reviewed-on: https://chromium-review.googlesource.com/574527
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46732}

v8/v8@182caaf
  Do not track transitions for built-in objects.

  Objects created during bootstrapping do not need
  a transition tree except for elements kind transitions.

  Bug: v8:6596
  Change-Id: I237b8b2792f201336e1c9731c815095dd06bc182
  Reviewed-on: https://chromium-review.googlesource.com/571750
  Reviewed-by: Igor Sheludko <ishell@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46693}

Fixes: #14171
Refs: #14345

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@hashseed hashseed deleted the rehash branch August 3, 2017 05:10
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@ofrobots ofrobots ofrobots approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@targos targos Awaiting requested review from targos

Assignees
No one assigned
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Re-enabling V8 snapshots
8 participants
@hashseed @MylesBorins @ofrobots @targos @bnoordhuis @addaleax @refack @nodejs-github-bot