src: fix fully-static and large-pages combination by suresh-srinivas · Pull Request #23964 · nodejs/node

suresh-srinivas · 2018-10-30T00:08:38Z

Fixes: #23906
Refs: #22079

This change to ld.implicit.script moves libc static code to
.lpstub area and avoids the issue detailed in 23906

Quick performance comparision on web-tooling shows 3%
improvement for the combination over fully-static

                                 fully-static-lp   fully-static
  cycles                       376,235,487,455  390,007,877,315
  instructions                 700,341,146,973  714,773,201,182
  itlb_misses_walk_completed        20,654,246       28,908,381
  itlb_misses_walk_completed_4k     19,884,666       28,865,118
  itlb_misses_walk_completed_2m_4m     769,391           43,251
  Score                                   9.13             8.86

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

suresh-srinivas · 2018-10-30T00:35:24Z

This doesnt seem to be related to this checkin. ENOSPC? @addaleax @refack @lundibundi could one of you help?

=== release test-fseventwrap ===
Path: async-hooks/test-fseventwrap
internal/fs/watchers.js:173
    throw error;
Error: ENOSPC: System limit for number of file watchers reached, watch '/home/travis/build/nodejs/node/test/async-hooks/test-fseventwrap.js'
    at FSWatcher.start (internal/fs/watchers.js:165:26)
    at Object.watch (fs.js:1274:11)
    at Object.<anonymous> (/home/travis/build/nodejs/node/test/async-hooks/test-fseventwrap.js:16:20)
    at Module._compile (internal/modules/cjs/loader.js:707:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:718:10)
    at Module.load (internal/modules/cjs/loader.js:605:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:544:12)
    at Function.Module._load (internal/modules/cjs/loader.js:536:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:760:12)
    at startup (internal/bootstrap/node.js:308:19)
Command: out/Release/node /home/travis/build/nodejs/node/test/async-hooks/test-fseventwrap.js
[06:56|% 100|+ 2453|-   1]: Done
make[1]: *** [jstest] Error 1
make: *** [test] Error 2

refack · 2018-10-30T01:51:11Z

 === release test-fseventwrap ===
 Path: async-hooks/test-fseventwrap 
 internal/fs/watchers.js:173
   throw error;

This is a Travis flake (funny coincidence, I was just looking at a WIP for a fix in the next tab #22589)
CI: https://ci.nodejs.org/job/node-test-pull-request/18221/

refack

RSLGTM

bnoordhuis

LGTM % nit.

I'm curious though what the ramifications are of linking in libc.a with non-static builds. Did you test that still works?

src/large_pages/ld.implicit.script

suresh-srinivas · 2018-10-30T17:18:03Z

@bnoordhuis this change has been tested on dynamic link builds where libc.a is not linked and this is a noop. What this rule in the linker script is doing is if libc.a is linked in, it places it in the .lpstub section outside the .text.

suresh-srinivas · 2018-10-30T17:59:40Z

Looks like I messed something up when I try to resolve a conflict. I dont know how those other commits got in here. I think I have fixed it following the contributors guide.

refack · 2018-10-30T18:17:28Z

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

suresh-srinivas · 2018-10-30T21:22:08Z

@refack the test failure on Ubuntu 16.04 ARM is not related to this checkin

https://ci.nodejs.org/job/node-test-commit-arm/nodes=ubuntu1604-arm64/lastFailedBuild/testReport/junit/(root)/test/parallel_test_fs_readfile/

Fixes: nodejs#23906 Refs: nodejs#22079 This change to ld.implicit.script moves libc static code to .lpstub area and avoids the issue detailed in 23906 Quick performance comparision on web-tooling shows 3% improvement for the combination over fully-static cycles 376,235,487,455 390,007,877,315 instructions 700,341,146,973 714,773,201,182 itlb_misses_walk_completed 20,654,246 28,908,381 itlb_misses_walk_completed_4k 19,884,666 28,865,118 itlb_misses_walk_completed_2m_4m 769,391 43,251 Score 9.13 8.86

refack · 2018-10-30T22:55:03Z

Probably. I've triggered a rerun of that platform: https://ci.nodejs.org/job/node-test-pull-request/18243/
If it doesn't fail again we'll mark it as "flaky".

suresh-srinivas · 2018-10-31T19:05:56Z

There is a PR 23861 to backport the original code to v10.x-staging. Should I raise another PR for this fix or can it be cherry picked as part of that?

refack · 2018-10-31T20:53:01Z

Since #23861 is still open, IMHO it's best to streamline this into #23861, once this lands.

refack · 2018-11-01T02:27:34Z

Landed in d32b5bd

Fixes: nodejs#23906 Refs: nodejs#22079 This change to ld.implicit.script moves libc static code to .lpstub area and avoids the issue detailed in 23906 Quick performance comparision on web-tooling shows 3% improvement for the combination over fully-static cycles 376,235,487,455 390,007,877,315 instructions 700,341,146,973 714,773,201,182 itlb_misses_walk_completed 20,654,246 28,908,381 itlb_misses_walk_completed_4k 19,884,666 28,865,118 itlb_misses_walk_completed_2m_4m 769,391 43,251 Score 9.13 8.86 PR-URL: nodejs#23964 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

suresh-srinivas · 2018-11-01T02:46:57Z

Landed in d32b5bd

Thanks @refack @bnoordhuis . Appreciate your support and patience in getting this fixed.

Fixes: #23906 Refs: #22079 This change to ld.implicit.script moves libc static code to .lpstub area and avoids the issue detailed in 23906 Quick performance comparision on web-tooling shows 3% improvement for the combination over fully-static cycles 376,235,487,455 390,007,877,315 instructions 700,341,146,973 714,773,201,182 itlb_misses_walk_completed 20,654,246 28,908,381 itlb_misses_walk_completed_4k 19,884,666 28,865,118 itlb_misses_walk_completed_2m_4m 769,391 43,251 Score 9.13 8.86 PR-URL: #23964 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins · 2018-11-27T07:11:56Z

I've gone ahead and backported to v10.x. I've opted to mark both #22079 and this PR as don't land on 8.x. If someone doesn't agree with that please feel free to chime in or open a backport

Fixes: #23906 Refs: #22079 This change to ld.implicit.script moves libc static code to .lpstub area and avoids the issue detailed in 23906 Quick performance comparision on web-tooling shows 3% improvement for the combination over fully-static cycles 376,235,487,455 390,007,877,315 instructions 700,341,146,973 714,773,201,182 itlb_misses_walk_completed 20,654,246 28,908,381 itlb_misses_walk_completed_4k 19,884,666 28,865,118 itlb_misses_walk_completed_2m_4m 769,391 43,251 Score 9.13 8.86 PR-URL: #23964 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

suresh-srinivas · 2018-11-27T18:05:41Z

I've gone ahead and backported to v10.x. I've opted to mark both #22079 and this PR as don't land on 8.x. If someone doesn't agree with that please feel free to chime in or open a backport

Thank you @MylesBorins for the backport to v10.x. Agree on your suggestion for don't land on 8.x

Fixes: #23906 Refs: #22079 This change to ld.implicit.script moves libc static code to .lpstub area and avoids the issue detailed in 23906 Quick performance comparision on web-tooling shows 3% improvement for the combination over fully-static cycles 376,235,487,455 390,007,877,315 instructions 700,341,146,973 714,773,201,182 itlb_misses_walk_completed 20,654,246 28,908,381 itlb_misses_walk_completed_4k 19,884,666 28,865,118 itlb_misses_walk_completed_2m_4m 769,391 43,251 Score 9.13 8.86 PR-URL: #23964 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 30, 2018

refack approved these changes Oct 30, 2018

View reviewed changes

bnoordhuis approved these changes Oct 30, 2018

View reviewed changes

src/large_pages/ld.implicit.script Outdated Show resolved Hide resolved

suresh-srinivas force-pushed the fully-static-large-pages branch from 0ea48bd to 887e1f2 Compare October 30, 2018 18:05

suresh-srinivas force-pushed the fully-static-large-pages branch from 887e1f2 to 98fb681 Compare October 30, 2018 18:08

refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 30, 2018

suresh-srinivas force-pushed the fully-static-large-pages branch from 98fb681 to f47ab8f Compare October 30, 2018 21:57

refack closed this Nov 1, 2018

refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 1, 2018

BridgeAR mentioned this pull request Nov 14, 2018

Release proposal: v11.2 #24350

Merged

This was referenced Nov 15, 2018

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#232

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-android-nodejs#249

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#233

Closed

This was referenced Nov 16, 2018

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#234

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#235

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#236

Closed

MylesBorins added lts-watch-v8.x and removed lts-watch-v8.x labels Nov 27, 2018

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

BethGriggs added backported-to-v10.x and removed lts-watch-v10.x labels Jul 9, 2019

Uh oh!

Conversation

suresh-srinivas commented Oct 30, 2018 • edited by refack Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

suresh-srinivas commented Oct 30, 2018

Uh oh!

refack commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

suresh-srinivas commented Oct 30, 2018

Uh oh!

suresh-srinivas commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Oct 30, 2018

Uh oh!

suresh-srinivas commented Oct 30, 2018

Uh oh!

refack commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suresh-srinivas commented Oct 31, 2018

Uh oh!

refack commented Oct 31, 2018

Uh oh!

refack commented Nov 1, 2018

Uh oh!

suresh-srinivas commented Nov 1, 2018

Uh oh!

MylesBorins commented Nov 27, 2018

Uh oh!

suresh-srinivas commented Nov 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

suresh-srinivas commented Oct 30, 2018 •

edited by refack

Loading

refack commented Oct 30, 2018 •

edited

Loading

suresh-srinivas commented Oct 30, 2018 •

edited

Loading

refack commented Oct 30, 2018 •

edited

Loading