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

[v12.x] deps: V8: cherry-pick 548f6c81d424 #33553

Closed
wants to merge 1 commit into from

Conversation

dominykas
Copy link
Member

@dominykas dominykas commented May 25, 2020


Original commit message:

[runtime] Don't track transitions for certainly detached maps

Previously such maps were marked as prototype, but that has bad
performance / memory characteristics if objects are used as
dictionaries.

Bug: b:148346655, v8:10339
Change-Id: I287c5664c8b7799a084669aaaffe3affcf73e95f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2179322
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67537}

Refs: v8/v8@548f6c8

PR-URL: #33484
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Matheus Marchini mat@mmarchini.me
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Original commit message:

    [runtime] Don't track transitions for certainly detached maps

    Previously such maps were marked as prototype, but that has bad
    performance / memory characteristics if objects are used as
    dictionaries.

    Bug: b:148346655, v8:10339
    Change-Id: I287c5664c8b7799a084669aaaffe3affcf73e95f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2179322
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Commit-Queue: Toon Verwaest <verwaest@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67537}

Refs: v8/v8@548f6c8

PR-URL: nodejs#33484
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v12.x v8 engine Issues and PRs related to the V8 dependency. labels May 25, 2020
@@ -34,7 +34,7 @@

# Reset this number to 0 on major V8 upgrades.
# Increment by one for each non-official patch applied to deps/v8.
'v8_embedder_string': '-node.37',
'v8_embedder_string': '-node.38',
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 line had a conflict when cherrypicking, but I think this just needs an increment, right?

@addaleax addaleax changed the title deps: V8: cherry-pick 548f6c81d424 [v12.x] deps: V8: cherry-pick 548f6c81d424 May 25, 2020
@@ -646,7 +646,7 @@ Map Map::FindRootMap(Isolate* isolate) const {
// Initial map always owns descriptors and doesn't have unused entries
// in the descriptor array.
DCHECK(result.owns_descriptors());
DCHECK_EQ(result.NumberOfOwnDescriptors(),
DCHECK_LE(result.NumberOfOwnDescriptors(),
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 line had a conflict (the context above it has changed) - I only changed the affected line.

@@ -1687,15 +1687,15 @@ void Map::ConnectTransition(Isolate* isolate, Handle<Map> parent,
child->may_have_interesting_symbols());
if (!parent->GetBackPointer().IsUndefined(isolate)) {
parent->set_owns_descriptors(false);
} else {
} else if (!parent->IsDetached(isolate)) {
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 line had a conflict (the context below it has changed) - I only changed the affected line.

@@ -2126,7 +2120,7 @@ Handle<Map> Map::TransitionToDataProperty(Isolate* isolate, Handle<Map> map,
StoreOrigin store_origin) {
RuntimeCallTimerScope stats_scope(
isolate, *map,
map->is_prototype_map()
map->IsDetached(isolate)
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 line had a conflict (the context above it has changed) - I only changed the affected line.

@dominykas dominykas marked this pull request as ready for review May 25, 2020 12:46
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 25, 2020

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 25, 2020
@ashupro
Copy link

ashupro commented Jun 1, 2020

@targos @dominykas Could you help us understand, To which version of Nodejs the fix will go?

@dominykas
Copy link
Member Author

@ashupro we will not know which exact version this will be released in until this actually lands in a release, as the version is determined by semver, but this PR is for the 12.x release line. I would assume this will not go out as part of tomorrow's (or soon after) security release, so I'd guess look out for something towards the end of June?

@targos
Copy link
Member

targos commented Jun 2, 2020

This will probably land in the next non-security v12.x release, on 2020-06-16

@ex1st ex1st mentioned this pull request Jun 9, 2020
@codebytere
Copy link
Member

Landed in af95bd7

@codebytere codebytere closed this Jun 9, 2020
codebytere pushed a commit that referenced this pull request Jun 9, 2020
Original commit message:

    [runtime] Don't track transitions for certainly detached maps

    Previously such maps were marked as prototype, but that has bad
    performance / memory characteristics if objects are used as
    dictionaries.

    Bug: b:148346655, v8:10339
    Change-Id: I287c5664c8b7799a084669aaaffe3affcf73e95f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2179322
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Commit-Queue: Toon Verwaest <verwaest@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67537}

Refs: v8/v8@548f6c8

PR-URL: #33484
Backport-PR-URL: #33553
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@dominykas dominykas deleted the fix-32049-on-v12.x branch June 10, 2020 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. 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