FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: backport rehash strings after deserialization by hashseed · Pull Request #14345 · nodejs/node · GitHub

/ node Public

deps: backport rehash strings after deserialization - #14345

Closed
hashseed wants to merge 1 commit into
nodejs:masterfrom
hashseed:rehash
ConversationCommits (1)ChecksFiles changed
Closed

deps: backport rehash strings after deserialization#14345
hashseed wants to merge 1 commit into
nodejs:masterfrom
hashseed:rehash

Conversation

hashseed commented Jul 18, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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 added the v8 engine Issues and PRs related to the V8 dependency. label Jul 18, 2017

hashseed commented Jul 18, 2017

Copy link
Copy Markdown
Member Author

@fhinkel @MylesBorins @ofrobots

MylesBorins requested review from ofrobots and targos July 18, 2017 15:37

MylesBorins commented Jul 18, 2017

Copy link
Copy Markdown
Contributor

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 commented Jul 18, 2017

Copy link
Copy Markdown
Contributor

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

MylesBorins commented Jul 18, 2017

Copy link
Copy Markdown
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 mentioned this pull request Jul 18, 2017
Closed
8 tasks

ofrobots commented Jul 18, 2017

Copy link
Copy Markdown
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 commented Jul 18, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Updated PR:

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

MylesBorins commented Jul 18, 2017

Copy link
Copy Markdown
Contributor

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

hashseed commented Jul 19, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

I just rebased onto b61cab2.

hashseed commented Jul 20, 2017

Copy link
Copy Markdown
Member Author

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

ofrobots commented Jul 20, 2017

Copy link
Copy Markdown
Contributor

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

targos commented Jul 20, 2017

Copy link
Copy Markdown
Member

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

hashseed commented Jul 20, 2017

Copy link
Copy Markdown
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 changed the title v8: rehash strings after deserialization deps: backport rehash strings after deserialization. Jul 20, 2017
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@{#46693}

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

hashseed commented Jul 20, 2017

Copy link
Copy Markdown
Member Author

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

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

refack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread configure
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'

refack Jul 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Optional: use b(want_snapshots)

hashseed Jul 20, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

refack Jul 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ack.

bnoordhuis approved these changes Jul 20, 2017
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, thanks.

Comment thread deps/v8/src/snapshot/deserializer.cc
// 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));

bnoordhuis Jul 20, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Jul 20, 2017

Copy link
Copy Markdown
Member Author

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

bnoordhuis commented Jul 20, 2017

Copy link
Copy Markdown
Member

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

hashseed commented Jul 20, 2017

Copy link
Copy Markdown
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 commented Jul 28, 2017

Copy link
Copy Markdown
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 commented Jul 28, 2017

Copy link
Copy Markdown
Member

Landed in 8dce05f 🎉

addaleax closed this Jul 28, 2017
addaleax pushed a commit that referenced this pull request Jul 28, 2017
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 commented Jul 28, 2017

Copy link
Copy Markdown
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 commented Jul 28, 2017

Copy link
Copy Markdown
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 commented Jul 28, 2017

Copy link
Copy Markdown
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 commented Jul 28, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

6.0 backport included as 2ae2874

hashseed commented Jul 28, 2017

Copy link
Copy Markdown
Member Author

Thanks Myles!

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Aug 1, 2017
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@{#46693}

Fixes: nodejs#14171
PR-URL: nodejs#14345
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Aug 1, 2017
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@{#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
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
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 deleted the rehash branch August 3, 2017 05:10
jasnell mentioned this pull request Oct 30, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
targos Awaiting requested review from targos
+2 more reviewers
refack refack approved these changes
ofrobots ofrobots approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL