Linux large pages - after rebase #22079

uttampawar · 2018-08-02T00:29:24Z

Support under Linux to map the text segment of node into 2M pages.
As requested in PR #21064, I've re-base the huge page support code with master branch.

Make test results show no failures.
$ make test
[03:13|% 100|+ 2363|- 0]: Done

Checklist

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

uttampawar · 2018-08-02T00:30:07Z

@gireeshpunathil I've rebased the code. Please let me know if I'm missing anything. Thanks.

gireeshpunathil · 2018-08-02T04:42:07Z

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

gireeshpunathil · 2018-08-02T04:47:21Z

@uttampawar - thanks. Given that this is opened in lieu of #21064,

can we have that one closed?
can you confirm that the code is same as Large Page Support for Code Issue: 16198 #21064 ?
can you please change the title - probably you meant Linux Large Pages ?

uttampawar · 2018-08-02T15:59:55Z

@gireeshpunathil The code is exactly the same. And yes #21064 can be closed.

uttampawar · 2018-08-02T16:10:31Z

Last change (addition of new line) is the only difference between original code and the one I submitted.

gireeshpunathil · 2018-08-03T10:35:40Z

@addaleax @Fishrock123 @gabrielschulhof - this is a clone of #21064 (resubmitted here due to author unavailable I guess). Given that you had reviewed that, will you please have a look?

Also the feature under this PR is exercised only if configured with --use-largepages (that too only in Linux) so running CI does not cover that, instead only helps making sure it does not break anything. How do we go about this in general? custom CI that builds with custom params? /cc @Trott

addaleax

CI with --use-largepages: https://ci.nodejs.org/job/node-test-commit/20173/

addaleax · 2018-08-03T10:48:19Z

src/node.cc

@@ -3806,6 +3807,15 @@ int Start(int argc, char** argv) {

  CHECK_GT(argc, 0);

+#ifdef NODE_ENABLE_LARGE_CODE_PAGES
+  if (node::IsLargePagesEnabled()) {
+    if ((node::MapStaticCodeToLargePages()) != 0) {


style nit: redundant ()?

addaleax · 2018-08-03T10:50:43Z

src/node_large_page.cc

+  iss >> permission;
+
+  if (permission.compare("r-xp") == 0) {
+    start = reinterpret_cast<unsigned int64_t>(&__nodetext);


style nit: uint64_t

addaleax · 2018-08-03T10:50:54Z

src/node_large_page.cc

+  if (permission.compare("r-xp") == 0) {
+    start = reinterpret_cast<unsigned int64_t>(&__nodetext);
+    char* from = reinterpret_cast<char* >(hugepage_align_up(start));
+    char* to = reinterpret_cast<char* >(hugepage_align_down(end));


style nit: no space before >

addaleax · 2018-08-03T10:51:53Z

src/node_large_page.cc

+}
+
+inline int64_t hugepage_align_up(int64_t addr) {
+    const size_t hps = 2L * 1024 * 1024;


Given that this line occurs a number of times, maybe it makes sense to move it out of the functions?

Also, style nit: 2 spaces of indentation

addaleax · 2018-08-03T10:55:25Z

src/node_large_page.cc

+__attribute__((__aligned__(2 * 1024 * 1024)))
+__attribute__((__noinline__))
+__attribute__((__optimize__("2")))
+MoveTextRegionToLargePages(struct text_region r) {


style nit: const text_region& r (struct is not necessary)?

src/node_large_page.cc

addaleax · 2018-08-03T11:01:13Z

src/node_large_page.cc

+    return -1;
+  }
+
+  if (r.from > reinterpret_cast<void* > (&MoveTextRegionToLargePages))


ditto here, no space before/after the cast’s >

addaleax · 2018-08-03T11:01:43Z

src/node_large_page.cc

+int MapStaticCodeToLargePages() {
+  struct text_region r = FindNodeTextRegion();
+  if (r.found_text_region == false) {
+    fprintf(stderr, "Hugepages WARNING: failed to find text region \n");


There’s an extra space at the end of the message here

addaleax · 2018-08-03T11:02:15Z

src/node_large_page.h

+
+namespace node {
+  bool IsLargePagesEnabled();
+  int MapStaticCodeToLargePages();


style nit: no indentation inside namespaces, blank lines/before after the namespace lines

addaleax · 2018-08-03T11:04:25Z

src/node_large_page.cc

+//    If successful copy the code there and unmap the original region.
+
+extern char __executable_start;
+extern char __etext;


Where are these two used?

Trott · 2018-08-03T17:43:35Z

I'm not sure if this has security implications, but out of caution: @nodejs/security-wg

uttampawar · 2018-08-06T16:31:28Z

@addaleax Thanks for the review. You are correct, this is re-submission since original author is on vacation. I'll address suggested changes.

uttampawar · 2018-08-09T16:33:28Z

From the test log I see "test-trace-event-promises" test failed as shown below,
not ok 1878 parallel/test-trace-event-promises
I don't see this failure on my "dev" machine. Is there anything can I do to address this issue? Is this failure due to my change?

addaleax · 2018-08-09T16:34:33Z

From the test log I see "test-trace-event-promises" test failed as shown below,
not ok 1878 parallel/test-trace-event-promises
I don't see this failure on my "dev" machine. Is there anything can I do to address this issue? Is this failure due to my change?

@uttampawar It’s unrelated (was due to a broken master branch), and it will go away after rebasing :)

uttampawar · 2018-08-13T17:59:22Z

@addaleax Do I need to re-base again and resubmit?

lundibundi · 2018-08-13T18:41:23Z

@uttampawar yeah, just rebase on current master and force push to your branch (uttampawar:intel-large-pages), no need to resubmit, GitHub will update this PR for you.

lundibundi · 2018-08-13T18:46:09Z

configure

+    action='store_true',
+    dest='node_use_large_pages',
+    help='build with Large Pages support (enabled only for Linux).' +
+         '(Needs Linux kernel >= 2.6.38 with Transparent Huge pages enabled)')


Just a nit 'Transparent Huge pages' -> 'Transparent Huge Pages'
Also, maybe
'build with Large Pages support. This feature is supported only on Linux kernel >= 2.6.38 with Transparent Huge Pages enabled'?

lundibundi · 2018-08-13T18:48:58Z

configure

@@ -977,6 +983,9 @@ def configure_node(o):
  else:
    o['variables']['node_use_dtrace'] = 'false'

+  use_large_pages = (flavor == 'linux' and options.node_use_large_pages)


I'd suggest to raise error (example here https://github.com/nodejs/node/blob/0a4ace24d5207ad3c86298a7159c782e6a289781/configure#L951-L953) instead of silently ignoring the option, what do you think?

@lundibundi I thin k it's a good idea. Failing early will help catch a build system error when Huge pages are not supported.
@addaleax @jasnell Any other opinions?

@uttampawar
I think we can go ahead and implement it this way considering

it is done this way in other places (see code below your change, enable-lto, dtrace etc)

considered a good change at least by me, you and @gireeshpunathil.

lundibundi · 2018-08-13T18:52:45Z

src/node_large_page.cc

+    return -1;
+  }
+
+  if (r.from > reinterpret_cast<void*> (&MoveTextRegionToLargePages))


Just a nit: unnecessary space after reinterpret_cast<void*>

Still here https://github.com/nodejs/node/pull/22079/files#r209719772. =)

uttampawar · 2018-08-14T00:28:58Z

@addaleax @lundibundi Finished re-base of large pages support patch.
I don't see any errors after rebasing as shown below,
$ make test
[03:24|% 100|+ 2365|- 0]: Done

Also, "make test-ci" shows no failures.

uttampawar · 2018-08-14T16:46:30Z

The build log says failure due to "no space left" error. See details below,

not ok 2294 sequential/test-fs-watch

duration_ms: 0.111
severity: fail
exitcode: 1
stack: |-
internal/fs/watchers.js:170
throw error;
^

Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.0/watch.txt'
    at FSWatcher.start (internal/fs/watchers.js:164:26)
    at Object.watch (fs.js:1255:11)
    at Object.<anonymous> (/home/travis/build/nodejs/node/test/sequential/test-fs-watch.js:48:22)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:266:19)

lundibundi · 2018-08-25T17:51:18Z

@uttampawar could you please rebase once again, just to be sure this PR has all of the updates from master (there were problems with the CI that has been fixed on master).

Ping @addaleax @gabrielschulhof @gireeshpunathil. Also, wdyt of #22079 (comment).

gireeshpunathil · 2018-08-26T02:07:39Z

raising exception on irrelevant platforms looks like a good option to me.

lundibundi · 2018-09-03T05:47:13Z

ping @uttampawar, PTAL #22079 (comment).

uttampawar · 2018-09-10T22:26:03Z

@lundibundi Rebased it.

uttampawar · 2018-09-10T23:31:40Z

@gireeshpunathil @addaleax @gabrielschulhof Can someone start CI with --use-largepages option please?

gireeshpunathil · 2018-09-11T05:24:15Z

CI: https://ci.nodejs.org/job/node-test-commit/21354/

addaleax

Just making sure this doesn’t land without having addressed the outstanding comments (since this might look like it could be ready, judging from the one approval and CI)

gireeshpunathil · 2018-09-11T10:34:47Z

and CI)

but why doesn't the CI result appear here? @addaleax - I started a CI few hours back (please see the link above), it's result does not seem to be available in this page: either as a summary or details. I am just wondering whether I ran it correctly or not, will you please have a look?

PR-URL: #22079 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

suresh-srinivas · 2018-10-22T18:47:47Z

Should this be backported to v10.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Thanks, yes we will work on raising a PR @uttampawar

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

Backport-PR-URL: nodejs#23861 PR-URL: nodejs#22079 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Backport-PR-URL: #23861 PR-URL: #22079 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Backport-PR-URL: #23861 PR-URL: #22079 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Backport-PR-URL: #23861 PR-URL: #22079 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 2, 2018

uttampawar changed the title ~~Intel large pages - after rebase~~ Linux large pages - after rebase Aug 2, 2018

gireeshpunathil approved these changes Aug 3, 2018

View reviewed changes

addaleax reviewed Aug 3, 2018

View reviewed changes

lundibundi reviewed Aug 13, 2018

View reviewed changes

uttampawar force-pushed the intel-large-pages branch from 0a4ace2 to 21bc4a1 Compare August 14, 2018 00:26

uttampawar force-pushed the intel-large-pages branch from 21bc4a1 to 0e30f77 Compare September 10, 2018 22:23

addaleax previously requested changes Sep 11, 2018

View reviewed changes

jasnell pushed a commit that referenced this pull request Oct 21, 2018

src: initial large page (2M) support

6c8a96f

PR-URL: #22079 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

uttampawar mentioned this pull request Oct 24, 2018

[v10.x backport] src: initial large page (2M) support #23861

Closed

2 tasks

mmarchini mentioned this pull request Oct 29, 2018

Using Large Pages (2M) in Node for Performance #16198

Closed

uttampawar deleted the intel-large-pages branch October 29, 2018 18:12

suresh-srinivas mentioned this pull request Oct 30, 2018

src: fix fully-static and large-pages combination #23964

Closed

4 tasks

MylesBorins added backported-to-v8.x and removed backport-requested-v10.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

Linux large pages - after rebase #22079

Linux large pages - after rebase #22079

Conversation

uttampawar commented Aug 2, 2018

Checklist

uttampawar commented Aug 2, 2018

gireeshpunathil commented Aug 2, 2018

gireeshpunathil commented Aug 2, 2018 • edited Loading

uttampawar commented Aug 2, 2018

uttampawar commented Aug 2, 2018

gireeshpunathil commented Aug 3, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Aug 3, 2018

uttampawar commented Aug 6, 2018

uttampawar commented Aug 9, 2018 • edited Loading

addaleax commented Aug 9, 2018

uttampawar commented Aug 13, 2018

lundibundi commented Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uttampawar commented Aug 14, 2018 • edited Loading

uttampawar commented Aug 14, 2018

not ok 2294 sequential/test-fs-watch

lundibundi commented Aug 25, 2018

gireeshpunathil commented Aug 26, 2018

lundibundi commented Sep 3, 2018

uttampawar commented Sep 10, 2018

uttampawar commented Sep 10, 2018

gireeshpunathil commented Sep 11, 2018

addaleax left a comment

Choose a reason for hiding this comment

gireeshpunathil commented Sep 11, 2018

suresh-srinivas commented Oct 22, 2018

gireeshpunathil commented Aug 2, 2018 •

edited

Loading

uttampawar commented Aug 9, 2018 •

edited

Loading

lundibundi commented Aug 13, 2018 •

edited

Loading

uttampawar commented Aug 14, 2018 •

edited

Loading