Skip to content

Conversation

@hashseed
Copy link
Member

Original commit message:

Fix collection iterator preview with deleted entries We used to assume that we know the remaining entries returned by the iterator based on the current index. However, that is not accurate, since entries skipped by the current index could be deleted. In the new approach, we allocate conservatively and shrink the result. [email protected] Bug: v8:8433 Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8 Reviewed-on: https://chromium-review.googlesource.com/c/1325966 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Georg Neis <[email protected]> Cr-Commit-Position: refs/heads/master@{#57360} 

Refs: v8/v8@88f8fe1
Refs: #24053

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

@nodejs-github-bot
Copy link
Collaborator

@hashseed sadly an error occured when I tried to trigger a build :(

@nodejs-github-botnodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Nov 20, 2018
@targos
Copy link
Member

LGTM if you remove deps/v8/src/value-serializer.cc.orig ;)

Original commit message: Fix collection iterator preview with deleted entries We used to assume that we know the remaining entries returned by the iterator based on the current index. However, that is not accurate, since entries skipped by the current index could be deleted. In the new approach, we allocate conservatively and shrink the result. [email protected] Bug: v8:8433 Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8 Reviewed-on: https://chromium-review.googlesource.com/c/1325966 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Georg Neis <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#57360} Refs: v8/v8@88f8fe1
@hashseed
Copy link
MemberAuthor

LGTM if you remove deps/v8/src/value-serializer.cc.orig ;)

Thanks for spotting this. Fixed :)

@targos
Copy link
Member

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 22, 2018
@addaleax
Copy link
Member

@danbev
Copy link
Contributor

Re-run of failing node-test-commit-windows-fanned.

@targos
Copy link
Member

Landed in 70e2f9d

@targostargos closed this Nov 24, 2018
targos pushed a commit that referenced this pull request Nov 24, 2018
Original commit message: Fix collection iterator preview with deleted entries We used to assume that we know the remaining entries returned by the iterator based on the current index. However, that is not accurate, since entries skipped by the current index could be deleted. In the new approach, we allocate conservatively and shrink the result. [email protected] Bug: v8:8433 Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8 Reviewed-on: https://chromium-review.googlesource.com/c/1325966 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Georg Neis <[email protected]> Cr-Commit-Position: refs/heads/master@{#57360} Refs: v8/v8@88f8fe1 PR-URL: #24514 Refs: #24053 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Nov 24, 2018
Original commit message: Fix collection iterator preview with deleted entries We used to assume that we know the remaining entries returned by the iterator based on the current index. However, that is not accurate, since entries skipped by the current index could be deleted. In the new approach, we allocate conservatively and shrink the result. [email protected] Bug: v8:8433 Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8 Reviewed-on: https://chromium-review.googlesource.com/c/1325966 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Georg Neis <[email protected]> Cr-Commit-Position: refs/heads/master@{#57360} Refs: v8/v8@88f8fe1 PR-URL: #24514 Refs: #24053 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Original commit message: Fix collection iterator preview with deleted entries We used to assume that we know the remaining entries returned by the iterator based on the current index. However, that is not accurate, since entries skipped by the current index could be deleted. In the new approach, we allocate conservatively and shrink the result. [email protected] Bug: v8:8433 Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8 Reviewed-on: https://chromium-review.googlesource.com/c/1325966 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Georg Neis <[email protected]> Cr-Commit-Position: refs/heads/master@{#57360} Refs: v8/v8@88f8fe1 PR-URL: #24514 Refs: #24053 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Dec 4, 2018
Original commit message: Fix collection iterator preview with deleted entries We used to assume that we know the remaining entries returned by the iterator based on the current index. However, that is not accurate, since entries skipped by the current index could be deleted. In the new approach, we allocate conservatively and shrink the result. [email protected] Bug: v8:8433 Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8 Reviewed-on: https://chromium-review.googlesource.com/c/1325966 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Georg Neis <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#57360} Refs: v8/v8@88f8fe1 PR-URL: nodejs#24514 Refs: nodejs#24053 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
@BridgeARBridgeAR mentioned this pull request Dec 5, 2018
4 tasks
pullbot pushed a commit to shakir-abdo/node that referenced this pull request Dec 6, 2018
Original commit message: Fix collection iterator preview with deleted entries We used to assume that we know the remaining entries returned by the iterator based on the current index. However, that is not accurate, since entries skipped by the current index could be deleted. In the new approach, we allocate conservatively and shrink the result. [email protected] Bug: v8:8433 Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8 Reviewed-on: https://chromium-review.googlesource.com/c/1325966 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Georg Neis <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#57360} Refs: v8/v8@88f8fe1 PR-URL: nodejs#24514 Refs: nodejs#24053 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Original commit message: Fix collection iterator preview with deleted entries We used to assume that we know the remaining entries returned by the iterator based on the current index. However, that is not accurate, since entries skipped by the current index could be deleted. In the new approach, we allocate conservatively and shrink the result. [email protected] Bug: v8:8433 Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8 Reviewed-on: https://chromium-review.googlesource.com/c/1325966 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Georg Neis <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#57360} Refs: v8/v8@88f8fe1 PR-URL: nodejs#24514 Refs: nodejs#24053 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Original commit message: Fix collection iterator preview with deleted entries We used to assume that we know the remaining entries returned by the iterator based on the current index. However, that is not accurate, since entries skipped by the current index could be deleted. In the new approach, we allocate conservatively and shrink the result. [email protected] Bug: v8:8433 Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8 Reviewed-on: https://chromium-review.googlesource.com/c/1325966 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Georg Neis <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#57360} Refs: v8/v8@88f8fe1 PR-URL: nodejs#24514 Refs: nodejs#24053 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
@hashseedhashseed deleted the backmergepreview branch January 28, 2019 13:55
@BethGriggs
Copy link
Member

This change does not land cleanly on v10.x-staging. I've added the backport-requested-v10.x label, but feel free to swap to dont-land-on- if this change shouldn't land on v10.x.

addaleax pushed a commit to addaleax/node that referenced this pull request May 26, 2019
Original commit message: Fix collection iterator preview with deleted entries We used to assume that we know the remaining entries returned by the iterator based on the current index. However, that is not accurate, since entries skipped by the current index could be deleted. In the new approach, we allocate conservatively and shrink the result. [email protected] Bug: v8:8433 Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8 Reviewed-on: https://chromium-review.googlesource.com/c/1325966 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Georg Neis <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#57360} [The backport to v10.x resolves merge conflicts due to a different way of accessing the “hole” value in V8, different signatures of the `Handle` constructor and the `Shrink()` method, and neighbouring-line conflicts in the test file.] Refs: v8/v8@88f8fe1Fixes: nodejs#27882 PR-URL: nodejs#24514 Refs: nodejs#24053 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jun 6, 2019
Original commit message: Fix collection iterator preview with deleted entries We used to assume that we know the remaining entries returned by the iterator based on the current index. However, that is not accurate, since entries skipped by the current index could be deleted. In the new approach, we allocate conservatively and shrink the result. [email protected] Bug: v8:8433 Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8 Reviewed-on: https://chromium-review.googlesource.com/c/1325966 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Georg Neis <[email protected]> Cr-Commit-Position: refs/heads/master@{#57360} [The backport to v10.x resolves merge conflicts due to a different way of accessing the “hole” value in V8, different signatures of the `Handle` constructor and the `Shrink()` method, and neighbouring-line conflicts in the test file.] Refs: v8/v8@88f8fe1Fixes: #27882 Backport-PR-URL: #27894 PR-URL: #24514 Refs: #24053 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
@BethGriggsBethGriggs mentioned this pull request Jul 17, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.buildIssues and PRs related to build files or the CI.v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@hashseed@nodejs-github-bot@targos@addaleax@danbev@BethGriggs@joyeecheung@devsnek@Trott