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?

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

Comment threadsrc/large_pages/ld.implicit.script Outdated

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

suresh-srinivas force-pushed the fully-static-large-pages branch from 887e1f2 to 98fb681Compare 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 f47ab8fCompare 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!

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