GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — deps: V8: cherry-pick beebee4f80ff by santigimeno · Pull Request #37293 · nodejs/node

Conversation

@santigimeno

@santigimeno santigimeno commented Feb 9, 2021

Copy link
Copy Markdown
Member

It contains the following commits:

deps: V8: cherry-pick beebee4f80ff

Original commit message:

cpu-profiler: Use Handle version of SourcePositionTableIterator

The surrounding code can trigger an allocation through InliningStack
which can eventually end up allocating a line ends array.
    
This is fine as-is because the existing iterator code makes a copy
of the byte array. It just triggers the no_gc dcheck in debug mode.

Fixed: v8:10778
Change-Id: Ic8c502767ec6c3d3b1f5e84df60638bd2fc6be75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2339102
Auto-Submit: Peter Marshall <[email protected]>
Commit-Queue: Tobias Tebbi <[email protected]>
Reviewed-by: Tobias Tebbi <[email protected]>
Cr-Commit-Position: refs/heads/master@{#69247}

Refs: v8/v8@beebee4

test: add cpu-profiler-crash test

This test is added as it usually crashes without applying the v8 patch:
v8/v8@beebee4

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@santigimeno santigimeno self-assigned this Feb 9, 2021
@nodejs-github-bot nodejs-github-bot added v14.x v8 engine Issues and PRs related to the V8 dependency. labels Feb 9, 2021
@santigimeno
santigimeno force-pushed the santi/backport_v8_10778_bugfix branch from cde372c to af26f41 Compare February 9, 2021 15:12
@richardlau

richardlau commented Feb 9, 2021

Copy link
Copy Markdown
Member

Can you target the staging branch (v14.x-staging)? Does this also apply to 15.x/master?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@santigimeno
santigimeno changed the base branch from v14.x to v14.x-staging February 9, 2021 16:26
@santigimeno

santigimeno commented Feb 9, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Can you target the staging branch (v14.x-staging)?

Done

Does this also apply to 15.x/master?

No, the fix is already there though it would be great if it could be applied to 12.x at some point.

Thanks!

richardlau reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@richardlau richardlau added the lts-watch-v12.x label Feb 9, 2021
@richardlau

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
gengjiawen
gengjiawen requested changes Feb 20, 2021
View reviewed changes

@gengjiawen gengjiawen left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You forget update common.gypi in first commit.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@santigimeno
santigimeno force-pushed the santi/backport_v8_10778_bugfix branch from af26f41 to f582bd8 Compare February 21, 2021 19:57
@santigimeno

santigimeno commented Feb 21, 2021

Copy link
Copy Markdown
Member Author

@gengjiawen thanks for pointing that out! Just updated the PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gengjiawen
gengjiawen approved these changes Feb 22, 2021
View reviewed changes
juanarbol
juanarbol approved these changes Feb 22, 2021
View reviewed changes
@juanarbol

juanarbol commented Feb 22, 2021

Copy link
Copy Markdown
Member

I could work on the v12.x backport

santigimeno reacted with thumbs up emoji santigimeno reacted with heart emoji
All reactions
  • 👍 1 reaction
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@BethGriggs
BethGriggs force-pushed the v14.x-staging branch from df5efb9 to 292dffe Compare February 23, 2021 12:52
psmarshall and others added 2 commits March 1, 2021 10:42
@psmarshall @santigimeno
deps: V8: cherry-pick beebee4f80ff
78af1d4
Original commit message:
```
cpu-profiler: Use Handle version of SourcePositionTableIterator

The surrounding code can trigger an allocation through InliningStack
which can eventually end up allocating a line ends array.

This is fine as-is because the existing iterator code makes a copy
of the byte array. It just triggers the no_gc dcheck in debug mode.

Fixed: v8:10778
Change-Id: Ic8c502767ec6c3d3b1f5e84df60638bd2fc6be75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2339102
Auto-Submit: Peter Marshall <[email protected]>
Commit-Queue: Tobias Tebbi <[email protected]>
Reviewed-by: Tobias Tebbi <[email protected]>
Cr-Commit-Position: refs/heads/master@{#69247}
```

Refs: v8/v8@beebee4
@santigimeno
test: add cpu-profiler-crash test
7908e99
This test is added as it usually crashes without applying the v8 patch:
v8/v8@beebee4
@santigimeno
santigimeno force-pushed the santi/backport_v8_10778_bugfix branch from f582bd8 to 7908e99 Compare March 1, 2021 09:42
richardlau
richardlau approved these changes Mar 1, 2021
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Mar 1, 2021 •
edited by richardlau
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36421/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/3812/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Mar 2, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

richardlau pushed a commit that referenced this pull request Mar 2, 2021
@psmarshall @richardlau
deps: V8: cherry-pick beebee4f80ff
15ed780
Original commit message:
```
cpu-profiler: Use Handle version of SourcePositionTableIterator

The surrounding code can trigger an allocation through InliningStack
which can eventually end up allocating a line ends array.

This is fine as-is because the existing iterator code makes a copy
of the byte array. It just triggers the no_gc dcheck in debug mode.

Fixed: v8:10778
Change-Id: Ic8c502767ec6c3d3b1f5e84df60638bd2fc6be75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2339102
Auto-Submit: Peter Marshall <[email protected]>
Commit-Queue: Tobias Tebbi <[email protected]>
Reviewed-by: Tobias Tebbi <[email protected]>
Cr-Commit-Position: refs/heads/master@{#69247}
```

Refs: v8/v8@beebee4

PR-URL: #37293
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 2, 2021
@santigimeno @richardlau
test: add cpu-profiler-crash test
69057eb
This test is added as it usually crashes without applying the v8 patch:
v8/v8@beebee4

PR-URL: #37293
Refs: v8/v8@beebee4
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@richardlau

richardlau commented Mar 2, 2021

Copy link
Copy Markdown
Member

Landed in 292dffe...69057eb.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@richardlau richardlau closed this Mar 2, 2021
@santigimeno
santigimeno deleted the santi/backport_v8_10778_bugfix branch March 2, 2021 17:23
juanarbol pushed a commit to juanarbol/node that referenced this pull request Mar 2, 2021
@psmarshall @juanarbol
deps: V8: cherry-pick beebee4f80ff
8d3fc8d
Original commit message:
```
cpu-profiler: Use Handle version of SourcePositionTableIterator

The surrounding code can trigger an allocation through InliningStack
which can eventually end up allocating a line ends array.

This is fine as-is because the existing iterator code makes a copy
of the byte array. It just triggers the no_gc dcheck in debug mode.

Fixed: v8:10778
Change-Id: Ic8c502767ec6c3d3b1f5e84df60638bd2fc6be75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2339102
Auto-Submit: Peter Marshall <[email protected]>
Commit-Queue: Tobias Tebbi <[email protected]>
Reviewed-by: Tobias Tebbi <[email protected]>
Cr-Commit-Position: refs/heads/master@{#69247}
```

Refs: v8/v8@beebee4

PR-URL: nodejs#37293
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Mar 2, 2021
@santigimeno @juanarbol
test: add cpu-profiler-crash test
a6eae67
This test is added as it usually crashes without applying the v8 patch:
v8/v8@beebee4

PR-URL: nodejs#37293
Refs: v8/v8@beebee4
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@juanarbol juanarbol mentioned this pull request Mar 2, 2021
[v12.x backport] deps: V8: cherry-pick beebee4f80ff #37578
Closed
@juanarbol juanarbol added backport-requested-v12.x and removed lts-watch-v12.x labels Mar 2, 2021
richardlau pushed a commit that referenced this pull request Mar 16, 2021
@psmarshall @richardlau
deps: V8: cherry-pick beebee4f80ff
dfa04d9
Original commit message:
```
cpu-profiler: Use Handle version of SourcePositionTableIterator

The surrounding code can trigger an allocation through InliningStack
which can eventually end up allocating a line ends array.

This is fine as-is because the existing iterator code makes a copy
of the byte array. It just triggers the no_gc dcheck in debug mode.

Fixed: v8:10778
Change-Id: Ic8c502767ec6c3d3b1f5e84df60638bd2fc6be75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2339102
Auto-Submit: Peter Marshall <[email protected]>
Commit-Queue: Tobias Tebbi <[email protected]>
Reviewed-by: Tobias Tebbi <[email protected]>
Cr-Commit-Position: refs/heads/master@{#69247}
```

Refs: v8/v8@beebee4

PR-URL: #37293
Backport-PR-URL: #37578
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 16, 2021
@santigimeno @richardlau
test: add cpu-profiler-crash test
0f6d445
This test is added as it usually crashes without applying the v8 patch:
v8/v8@beebee4

PR-URL: #37293
Backport-PR-URL: #37578
Refs: v8/v8@beebee4
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@juanarbol juanarbol added backported-to-v12.x and removed backport-requested-v12.x labels Mar 16, 2021
@richardlau richardlau mentioned this pull request Mar 18, 2021
v12.22.0 proposal #37797
Merged
MylesBorins pushed a commit that referenced this pull request Apr 6, 2021
@psmarshall @MylesBorins
deps: V8: cherry-pick beebee4f80ff
e446d82
Original commit message:
```
cpu-profiler: Use Handle version of SourcePositionTableIterator

The surrounding code can trigger an allocation through InliningStack
which can eventually end up allocating a line ends array.

This is fine as-is because the existing iterator code makes a copy
of the byte array. It just triggers the no_gc dcheck in debug mode.

Fixed: v8:10778
Change-Id: Ic8c502767ec6c3d3b1f5e84df60638bd2fc6be75
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2339102
Auto-Submit: Peter Marshall <[email protected]>
Commit-Queue: Tobias Tebbi <[email protected]>
Reviewed-by: Tobias Tebbi <[email protected]>
Cr-Commit-Position: refs/heads/master@{#69247}
```

Refs: v8/v8@beebee4

PR-URL: #37293
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 6, 2021
@santigimeno @MylesBorins
test: add cpu-profiler-crash test
89046d7
This test is added as it usually crashes without applying the v8 patch:
v8/v8@beebee4

PR-URL: #37293
Refs: v8/v8@beebee4
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gengjiawen gengjiawen gengjiawen approved these changes
@richardlau richardlau richardlau approved these changes
@juanarbol juanarbol juanarbol approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@santigimeno santigimeno

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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@santigimeno @richardlau @nodejs-github-bot @juanarbol @gengjiawen @psmarshall
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.