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.

Sign up for GitHub

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

Jump to bottom

[deps] V8: cherry-pick 71736859756b2bd0444bdb0a87a #35205

Closed
wants to merge 4 commits into from
Closed

[deps] V8: cherry-pick 71736859756b2bd0444bdb0a87a #35205

wants to merge 4 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Sep 15, 2020

Original commit message:

[heap] Add large_object_threshold to AllocateRaw

This commit adds a check in Heap::AllocateRaw when setting the
large_object variable, when the AllocationType is of type kCode, to
take into account the size of the CodeSpace's area size.

The motivation for this change is that without this check it is
possible that size_in_bytes is less than 128, and hence not considered
a large object, but it might be larger than the available space
in code_space->AreaSize(), which will cause the object to be created
in the CodeLargeObjectSpace. This will later cause a segmentation fault
when calling the following chain of functions:

  if (!large_object) {
     MemoryChunk::FromHeapObject(heap_object)
         ->GetCodeObjectRegistry()
         ->RegisterNewlyAllocatedCodeObject(heap_object.address());
  }

We (Red Hat) ran into this issue when running Node.js v12.16.1 in
combination with yarn on aarch64 (this was the only architecture that
this happed on).

Bug: v8:10808
Change-Id: I0c396b0eb64bc4cc91d9a3be521254f3130eac7b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2390665
Commit-Queue: Ulan Degenbaev ulan@chromium.org
Reviewed-by: Ulan Degenbaev ulan@chromium.org
Cr-Commit-Position: refs/heads/master@{#69876}

Refs: v8/v8@7173685

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@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 Sep 15, 2020
@yselkowitz
Copy link

Is there a PR for 14 as well?

@danbev
Copy link
Contributor Author

danbev commented Sep 15, 2020

Is there a PR for 14 as well?

Not yet, but I'll create one tomorrow.

@targos
Copy link
Member

targos commented Sep 15, 2020

If this applies to the V8 version we have in master/v14.x, please only open a PR against master. It can then be backported to v12.x

@danbev
Copy link
Contributor Author

danbev commented Sep 15, 2020 •
edited

If this applies to the V8 version we have in master/v14.x, please only open a PR against master.

I can create a PR against master tomorrow 👍
The reason for opening this PR is that the patch needed some modifications to apply against the V8 version in 12.

@danbev danbev force-pushed the deps-v8-heap-allocateraw branch 2 times, most recently from 982be1f to c012d6d Compare September 16, 2020 04:46
@danbev
Copy link
Contributor Author

danbev commented Sep 22, 2020

V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/3380/

@danbev
Copy link
Contributor Author

danbev commented Sep 22, 2020

V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/3382/

@danbev danbev requested review from a team September 22, 2020 16:11
@danbev danbev removed request for a team September 22, 2020 16:55
@addaleax
Copy link
Member

@danbev Sorry to ask, but could you rebase this?

@danbev
Copy link
Contributor Author

danbev commented Sep 23, 2020

@danbev Sorry to ask, but could you rebase this?

No worries, I'll rebase shortly.

@danbev
[deps] V8: cherry-pick 71736859756b2bd0444bdb0a87a
a8287fe 
Original commit message:

   [heap] Add large_object_threshold to AllocateRaw

   This commit adds a check in Heap::AllocateRaw when setting the
   large_object variable, when the AllocationType is of type kCode, to
   take into account the size of the CodeSpace's area size.

   The motivation for this change is that without this check it is
   possible that size_in_bytes is less than 128, and hence not considered
   a large object, but it might be larger than the available space
   in code_space->AreaSize(), which will cause the object to be created
   in the CodeLargeObjectSpace. This will later cause a segmentation fault
   when calling the following chain of functions:

      if (!large_object) {
         MemoryChunk::FromHeapObject(heap_object)
             ->GetCodeObjectRegistry()
             ->RegisterNewlyAllocatedCodeObject(heap_object.address());
      }

   We (Red Hat) ran into this issue when running Node.js v12.16.1 in
   combination with yarn on aarch64 (this was the only architecture that
   this happed on).

   Bug: v8:10808
   Change-Id: I0c396b0eb64bc4cc91d9a3be521254f3130eac7b
   Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2390665
   Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
   Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
   Cr-Commit-Position: refs/heads/master@{#69876}

Refs: v8/v8@7173685
@danbev
squash: use ToObjectChecked instead of ToAddress()
5c214a3
@danbev
squash: use ToObjectChecked in CreateFillerObjectAt
11a0593
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 23, 2020 •
edited by richardlau

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

@richardlau
Copy link
Member

@danbev looks like the V8 CI fails for this PR.

@danbev
Copy link
Contributor Author

danbev commented Sep 23, 2020

@richardlau Thanks again! And sorry about using CI instead of testing locally but I'm running low on disk space on my machine.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2020
@danbev
Copy link
Contributor Author

danbev commented Sep 23, 2020

Re-build of V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/3395/

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@addaleax
Copy link
Member

Landed in 57564eb

@addaleax addaleax closed this Sep 23, 2020
addaleax pushed a commit that referenced this pull request Sep 23, 2020
@danbev @addaleax
deps: V8: cherry-pick 71736859756b2bd0444bdb0a87a
57564eb 
Original commit message:

    [heap] Add large_object_threshold to AllocateRaw

    This commit adds a check in Heap::AllocateRaw when setting the
    large_object variable, when the AllocationType is of type kCode, to
    take into account the size of the CodeSpace's area size.

    The motivation for this change is that without this check it is
    possible that size_in_bytes is less than 128, and hence not considered
    a large object, but it might be larger than the available space
    in code_space->AreaSize(), which will cause the object to be created
    in the CodeLargeObjectSpace. This will later cause a segmentation fault
    when calling the following chain of functions:

       if (!large_object) {
          MemoryChunk::FromHeapObject(heap_object)
              ->GetCodeObjectRegistry()
              ->RegisterNewlyAllocatedCodeObject(heap_object.address());
       }

    We (Red Hat) ran into this issue when running Node.js v12.16.1 in
    combination with yarn on aarch64 (this was the only architecture that
    this happed on).

    Bug: v8:10808
    Change-Id: I0c396b0eb64bc4cc91d9a3be521254f3130eac7b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2390665
    Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#69876}

Refs: v8/v8@7173685
PR-URL: #35205
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
@kasicka
Copy link

kasicka commented Jan 12, 2021

Was this backported to v14 as well?

@mhdawson
Copy link
Member

@kasicka looks like it was in #36074

@yselkowitz
Copy link

But that PR was closed without being merged?

@yselkowitz
Copy link

I can confirm that 14.15.4 does not contain the fix.

@kasicka kasicka mentioned this pull request Jan 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. 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.

None yet
8 participants
@danbev @yselkowitz @targos @addaleax @nodejs-github-bot @richardlau @kasicka @mhdawson