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

deps: backport rehash strings after deserialization #14345

Closed
wants to merge 1 commit into from

Conversation

hashseed
Copy link
Member

@hashseed hashseed commented Jul 18, 2017

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

@MylesBorins
Copy link
Contributor

@MylesBorins
Copy link
Contributor

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

@MylesBorins
Copy link
Contributor

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
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

Updated PR:

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

@MylesBorins
Copy link
Contributor

@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

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

@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
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.

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

@@ -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.

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.

@@ -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
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
Contributor

MylesBorins commented Jul 28, 2017

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
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
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
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
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
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.

Re-enabling V8 snapshots
8 participants