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: cherry-pick f4376ec801e1ded from V8 upstream #37225

Closed
wants to merge 2 commits into from
Closed

deps: cherry-pick f4376ec801e1ded from V8 upstream #37225

wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Feb 4, 2021 

Original commit message:
  [heap] Make maximum regular code object size a runtime value.

  Executable V8 pages include 3 reserved OS pages: one for the writable
  header and two as guards. On systems with 64k OS pages, the amount of
  allocatable space left for objects can then be quite smaller than the
  page size, only 64k for each 256k page.

  This means regular code objects cannot be larger than 64k, while the
  maximum regular object size is fixed to 128k, half of the page size. As
  a result code object never reach this limit and we can end up filling
  regular pages with few large code objects.

  To fix this, we change the maximum code object size to be runtime value,
  set to half of the allocatable space per page. On systems with 64k OS
  pages, the limit will be 32k.

  Alternatively, we could increase the V8 page size to 512k on Arm64 linux
  so we wouldn't waste code space. However, systems with 4k OS pages are
  more common, and those with 64k pages tend to have more memory available
  so we should be able to live with it.

  Bug: v8:10808
  Change-Id: I5d807e7a3df89f1e9c648899e9ba2f8e2648264c
  Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2460809
  Reviewed-by: Igor Sheludko <ishell@chromium.org>
  Reviewed-by: Georg Neis <neis@chromium.org>
  Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
  Commit-Queue: Pierre Langlois <pierre.langlois@arm.com>
  Cr-Commit-Position: refs/heads/master@{#70569}

Refs: nodejs/help#3202

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v14.x v8 engine Issues and PRs related to the V8 dependency. labels Feb 4, 2021
@danbev
Copy link
Contributor Author

danbev commented Feb 4, 2021 •
edited

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

mhdawson
mhdawson approved these changes Feb 4, 2021
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

sxa
sxa approved these changes Feb 4, 2021
View reviewed changes
Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

I've pulled over the three commits from this PR on top of my v14.15.4 and can confirm this does indeed fix the yarn segfault :-)

@targos
Copy link
Member

targos commented Feb 6, 2021

Is it possible to backport it to master first? I suppose it also affects v15.x

@danbev
Copy link
Contributor Author

danbev commented Feb 6, 2021

Is it possible to backport it to master first? I suppose it also affects v15.x

I think the original patch might apply to master but it does land cleanly against 14.15.x. Let me sort out the compilation errors in the test for this PR and then I'll take a look at creating a PR against master (on Monday).

@danbev
deps: cherry-pick f4376ec801e1ded from V8 upstream
ce35789 
    Original commit message:
      [heap] Make maximum regular code object size a runtime value.

      Executable V8 pages include 3 reserved OS pages: one for the writable
      header and two as guards. On systems with 64k OS pages, the amount of
      allocatable space left for objects can then be quite smaller than the
      page size, only 64k for each 256k page.

      This means regular code objects cannot be larger than 64k, while the
      maximum regular object size is fixed to 128k, half of the page size. As
      a result code object never reach this limit and we can end up filling
      regular pages with few large code objects.

      To fix this, we change the maximum code object size to be runtime value,
      set to half of the allocatable space per page. On systems with 64k OS
      pages, the limit will be 32k.

      Alternatively, we could increase the V8 page size to 512k on Arm64 linux
      so we wouldn't waste code space. However, systems with 4k OS pages are
      more common, and those with 64k pages tend to have more memory available
      so we should be able to live with it.

      Bug: v8:10808
      Change-Id: I5d807e7a3df89f1e9c648899e9ba2f8e2648264c
      Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2460809
      Reviewed-by: Igor Sheludko <ishell@chromium.org>
      Reviewed-by: Georg Neis <neis@chromium.org>
      Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
      Commit-Queue: Pierre Langlois <pierre.langlois@arm.com>
      Cr-Commit-Position: refs/heads/master@{#70569}

Refs: nodejs/help#3202
@danbev
test: add arm64 arch to test-worker-prof status
c44140c 
This commit adds arm64 to test-worker-prof status as this seems to be
flaky on arm64 in addition to arm.
@danbev danbev marked this pull request as ready for review February 8, 2021 12:24
@nodejs-github-bot
Copy link
Collaborator

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

@danbev danbev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 10, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@danbev
Copy link
Contributor Author

danbev commented Feb 15, 2021 •
edited

@targos this patch was included in the update of V8 to version 8.8.278.17 against master. Is it alright if I land this tomorrow on v14x-staging or should this be handled differently, just let me know? Thanks

danbev added a commit that referenced this pull request Feb 16, 2021
@danbev
deps: cherry-pick f4376ec801e1ded from V8 upstream
d1bf046 
    Original commit message:
      [heap] Make maximum regular code object size a runtime value.

      Executable V8 pages include 3 reserved OS pages: one for the writable
      header and two as guards. On systems with 64k OS pages, the amount of
      allocatable space left for objects can then be quite smaller than the
      page size, only 64k for each 256k page.

      This means regular code objects cannot be larger than 64k, while the
      maximum regular object size is fixed to 128k, half of the page size. As
      a result code object never reach this limit and we can end up filling
      regular pages with few large code objects.

      To fix this, we change the maximum code object size to be runtime value,
      set to half of the allocatable space per page. On systems with 64k OS
      pages, the limit will be 32k.

      Alternatively, we could increase the V8 page size to 512k on Arm64 linux
      so we wouldn't waste code space. However, systems with 4k OS pages are
      more common, and those with 64k pages tend to have more memory available
      so we should be able to live with it.

      Bug: v8:10808
      Change-Id: I5d807e7a3df89f1e9c648899e9ba2f8e2648264c
      Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2460809
      Reviewed-by: Igor Sheludko <ishell@chromium.org>
      Reviewed-by: Georg Neis <neis@chromium.org>
      Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
      Commit-Queue: Pierre Langlois <pierre.langlois@arm.com>
      Cr-Commit-Position: refs/heads/master@{#70569}

PR-URL: #37225
Refs: nodejs/help#3202
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danbev added a commit that referenced this pull request Feb 16, 2021
@danbev
test: add arm64 arch to test-worker-prof status
4c01304 
This commit adds arm64 to test-worker-prof status as this seems to be
flaky on arm64 in addition to arm.

PR-URL: #37225
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danbev
Copy link
Contributor Author

danbev commented Feb 16, 2021

Landed in d1bf046, and 4c01304.

@danbev danbev closed this Feb 16, 2021
@danbev danbev deleted the aarch64-yarn-14.x branch February 16, 2021 07:42
MylesBorins pushed a commit that referenced this pull request Apr 6, 2021
@danbev @MylesBorins
deps: cherry-pick f4376ec801e1ded from V8 upstream
ae1fa98 
    Original commit message:
      [heap] Make maximum regular code object size a runtime value.

      Executable V8 pages include 3 reserved OS pages: one for the writable
      header and two as guards. On systems with 64k OS pages, the amount of
      allocatable space left for objects can then be quite smaller than the
      page size, only 64k for each 256k page.

      This means regular code objects cannot be larger than 64k, while the
      maximum regular object size is fixed to 128k, half of the page size. As
      a result code object never reach this limit and we can end up filling
      regular pages with few large code objects.

      To fix this, we change the maximum code object size to be runtime value,
      set to half of the allocatable space per page. On systems with 64k OS
      pages, the limit will be 32k.

      Alternatively, we could increase the V8 page size to 512k on Arm64 linux
      so we wouldn't waste code space. However, systems with 4k OS pages are
      more common, and those with 64k pages tend to have more memory available
      so we should be able to live with it.

      Bug: v8:10808
      Change-Id: I5d807e7a3df89f1e9c648899e9ba2f8e2648264c
      Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2460809
      Reviewed-by: Igor Sheludko <ishell@chromium.org>
      Reviewed-by: Georg Neis <neis@chromium.org>
      Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
      Commit-Queue: Pierre Langlois <pierre.langlois@arm.com>
      Cr-Commit-Position: refs/heads/master@{#70569}

PR-URL: #37225
Refs: nodejs/help#3202
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 6, 2021
@danbev @MylesBorins
test: add arm64 arch to test-worker-prof status
6961be5 
This commit adds arm64 to test-worker-prof status as this seems to be
flaky on arm64 in addition to arm.

PR-URL: #37225
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@sxa sxa sxa approved these changes

@mhdawson mhdawson mhdawson approved these changes

@juanarbol juanarbol juanarbol approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@danbev @targos @nodejs-github-bot @jasnell @sxa @mhdawson @juanarbol