pgo: enabling pgo at configure #21596

octaviansoldea · 2018-06-29T23:26:00Z

This modification allows for compiling with profiled guided optimization (pgo) using the flags
--enable-pgo-generate and --enable-pgo-use.

Refs: #21583
Refs: #1409

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

This is a modification proposed towards enabling pgo compilation. From some preliminary results, I have the following data:

I have compared Node-DC-EIS and Ghost, and have obtained 3.7% and 3.8% improvements respectively. These numbers were validated with unpaired t-test. Moreover, I am collecting data regarding the Node.js benchmark suite and mentioned partial results in Refs: #21583. In this context, I would like to mention that assert and async manifest 3.17% and 3.92% improvements respectively.

The experiments were done on Intel(R) Xeon(R) Platinum 8180 CPU @ 2.50GHz. Nevertheless, the solution passes the tests, and also compiles for 32 bits, see also issue Refs: #1409.

mscdex · 2018-06-30T00:03:09Z

configure

+
+  if flavor == 'linux':
+    if options.enable_pgo_generate or options.enable_pgo_use:
+      if CC != 'gcc' or CXX != 'g++':


This isn't a very good check, especially since some distros support multiple gcc/g++ versions using different executable names. Perhaps it would be better to check for a valid gcc_version from try_check_compiler()?

Hello

Thank you for your feedback and suggestion. I will implement this asap.

@octaviansoldea

octaviansoldea · 2018-07-10T17:25:03Z

Dear Reviewers

Thank you for your valuable comments. I would like to ask if are there any additional comments or suggestions?

@octaviansoldea

jasnell · 2018-07-12T14:59:34Z

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

lundibundi · 2018-07-28T18:13:59Z

common.gypi

-            ],
-          },],
+            ],},
+          ],


I don't think you have to change these 2 lines, is this intentional?

Agree and tried to resolve it.

lundibundi · 2018-07-28T18:19:54Z

configure

@@ -892,7 +904,6 @@ def configure_mips(o):
  o['variables']['mips_arch_variant'] = options.mips_arch_variant
  o['variables']['mips_fpu_mode'] = options.mips_fpu_mode

-


Any reason to remove this blank? I'm pretty sure it's according to pep to separate top-level functions with 2 blanks.

Agree and tried to resolve it.

lundibundi · 2018-07-28T18:21:02Z

configure

@@ -955,7 +989,6 @@ def configure_node(o):
            ' or newer only.' % (version_checked_str))

  o['variables']['enable_lto'] = b(options.enable_lto)
-


Also here, any reason for the removal?

Agree and tried to resolve it.

lundibundi · 2018-07-28T18:24:18Z

configure

+    action="store_true",
+    dest="enable_pgo_generate",
+    help="Enable profiling with pgo of a binary. This feature is only available "
+         "on linux with gcc and g++.")


Probably worth adding a minimal version for gcc/g++ here (and below) too?

Agree and tried to implement the suggestion. Please see lines 962-998 in configure and the comment in the following.

I meant to include minimal gcc version in this comment to let people know it from help. Something like:
'This feature is only available on linux with gcc and g++ 5.4.1 or newer only.'. And the same for line 166.

octaviansoldea · 2018-08-01T18:17:13Z

Dear Reviewers

Thank you for the feedback. I will come back with changes asap.

@octaviansoldea

octaviansoldea · 2018-08-02T01:37:50Z

Dear Reviewers,

Thank you again for the very good feedback. In a trial to answer the suggestions, I have tried to resolve the problems related to lines and indentations. Moreover, regarding the checking of the compiler, I tried to make a uniform design with the lto code. In this context, the tests for the compiler are located between lines 962-998 in configure. Moreover, I have introduced the function check_gcc_version that is used for both pgo and lto. Of course, pgo is covered in both of generate and use cases. The reason for doing the checks in these lines is following the fact that more tests are done here, see also

if flavor == 'aix':
...

if target_arch in ('x86', 'x64', 'ia32', 'x32'):
...
elif options.enable_vtune_profiling:
...
else:
...

in lines 950-960.

Could you please tell if you have further suggestions? Is there anything I can do in order to help making the code better?

Thank you in advance,
@octaviansoldea

lundibundi

Thanks for your work 👍
Just a few nits.

lundibundi · 2018-08-02T22:00:09Z

configure

@@ -893,6 +905,16 @@ def configure_mips(o):
  o['variables']['mips_fpu_mode'] = options.mips_fpu_mode


+def check_gcc_version(version_checked):


Maybe gcc_version_ge? Though I'm fine with this too. (ge for greater-or-equals in python)

Agree and tried to resolve it.

lundibundi · 2018-08-02T22:04:17Z

configure

-          raise Exception(
-            'The option --enable-lto is supported for gcc and gxx %s'
-            ' or newer only.' % (version_checked_str))
+      if check_gcc_version(version_checked) == False:


if not check_gcc_version(version_checked):

Agree and tried to resolve it.

lundibundi · 2018-08-02T22:05:52Z

configure

+      if check_gcc_version(version_checked) == False:
+        version_checked_str = ".".join(map(str, version_checked))
+        raise Exception(
+          'The options --enable-pgo-generate and --enable-pgo-use options '


Duplicate 'options', should probably be 'The options --enable-pgo-generate and --enable-pgo-use '?

Agree and tried to resolve it.

lundibundi · 2018-08-02T22:09:48Z

configure

+    action="store_true",
+    dest="enable_pgo_generate",
+    help="Enable profiling with pgo of a binary. This feature is only available "
+         "on linux with gcc and g++.")


I meant to include minimal gcc version in this comment to let people know it from help. Something like:
'This feature is only available on linux with gcc and g++ 5.4.1 or newer only.'. And the same for line 166.

lundibundi · 2018-08-02T22:11:12Z

configure

+  if flavor == 'linux':
+    if options.enable_pgo_generate or options.enable_pgo_use:
+      version_checked = (5, 4, 1)
+      if check_gcc_version(version_checked) == False:


Also if not check_gcc_version(version_checked):

Agree and tried to resolve it.

lundibundi · 2018-08-05T09:40:25Z

This needs CI and some final reviews. @addaleax @bnoordhuis @mscdex

lundibundi

LGTM.

lundibundi · 2018-08-25T18:03:22Z

@octaviansoldea could you please rebase this PR on the current master?

Also, ping @jasnell @richardlau @bnoordhuis @addaleax.

jasnell

still lgtm

octaviansoldea · 2018-08-31T02:36:44Z

Dear Reviewers

Thank you for your messages. I will answer asap.

@octaviansoldea

This modification allows for compiling with profiled guided optimization (pgo) using the flags --enable-pgo-generate and --enable-pgo-use. Refs: nodejs#21583 Refs: nodejs#1409

lundibundi · 2018-09-02T15:24:27Z

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

addaleax · 2018-09-02T18:14:17Z

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

addaleax · 2018-09-03T12:29:16Z

Resume CI, again: https://ci.nodejs.org/job/node-test-pull-request/16985/

gabrielschulhof · 2018-09-04T22:47:49Z

The latter CI is actually green, so landing.

gabrielschulhof · 2018-09-04T22:50:15Z

Landed in 9be1555.

This modification allows for compiling with profiled guided optimization (pgo) using the flags --enable-pgo-generate and --enable-pgo-use. Refs: #21583 Refs: #1409 PR-URL: #21596 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

misterdjules · 2018-09-06T00:10:52Z

@octaviansoldea Is it correct that this PR does not enable Profile Guided Optimization by default? If so, is that the goal in the future and is there already a PR/issue where we can track that change/discussion?

targos · 2018-09-23T08:51:19Z

@addaleax You said in #22716 that this was breaking native addons. Do we need to revert the change?

lundibundi · 2018-09-25T17:27:27Z

@targos there has been an #22772 but it was decided that breakage was caused by some stale configs and native addons should be working fine.
Though @addaleax could you please confirm that there has not been any other problems related to this?

addaleax · 2018-09-25T17:31:11Z

@lundibundi @targos I can’t reproduce any breakage anymore. I guess that’s a pretty good sign but I’m not sure how to go back to the situation where I had the broken config.

targos · 2018-09-25T19:28:48Z

Should we try to land it again on v10?

octaviansoldea · 2018-09-26T01:13:11Z

Dear Reviewers, Hello,

First of all, please accept my apologies for a prolonged inactivity, it is related to personal issues.

@misterdjules the intention was to use pgo on demand only, i.e. not to enable it by default. Adding pgo by default means having a predefined workload for profiling, and, actually it would be good to ask the community if there is such an accepted software.

In this context, I would like to backport this PR to v10. However, I feel I would like to learn a bit more what is to be done, provided, this is acceptable, of course.

@addaleax I would like to ask for a bit of guidance for understanding what were the reasons not to land this on v10? Could you and colleagues, please, write a bit more comments on this?

Thank you in advance,
@octaviansoldea

addaleax · 2018-09-27T11:51:42Z

@octaviansoldea Sorry, it seems like this was caused by me using a stale config of some sort after trying to build addons on the v10.x-staging branch. We’ll land it on v10.x and see if something goes wrong.

This modification allows for compiling with profiled guided optimization (pgo) using the flags --enable-pgo-generate and --enable-pgo-use. Refs: #21583 Refs: #1409 PR-URL: #21596 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jun 29, 2018

octaviansoldea mentioned this pull request Jun 29, 2018

enabling pgo #21583

Closed

mscdex reviewed Jun 30, 2018

View reviewed changes

octaviansoldea force-pushed the enabling_pgo branch from fb27526 to 93d6e2f Compare July 3, 2018 00:25

richardlau approved these changes Jul 10, 2018

View reviewed changes

jasnell requested review from bnoordhuis and addaleax July 10, 2018 19:47

jasnell approved these changes Jul 12, 2018

View reviewed changes

octaviansoldea force-pushed the enabling_pgo branch from d1a8f83 to e515d6e Compare July 17, 2018 03:23

lundibundi reviewed Jul 28, 2018

View reviewed changes

lundibundi reviewed Aug 2, 2018

View reviewed changes

lundibundi approved these changes Aug 25, 2018

View reviewed changes

jasnell approved these changes Aug 27, 2018

View reviewed changes

build: enabling pgo at configure

97967da

This modification allows for compiling with profiled guided optimization (pgo) using the flags --enable-pgo-generate and --enable-pgo-use. Refs: nodejs#21583 Refs: nodejs#1409

octaviansoldea force-pushed the enabling_pgo branch from 0a16566 to 97967da Compare August 31, 2018 23:13

lundibundi mentioned this pull request Sep 2, 2018

Flaky addons/callback-scope/test-resolve-async #22668

Closed

gabrielschulhof closed this Sep 4, 2018

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

addaleax added the dont-land-on-v10.x label Sep 5, 2018

targos added this to Don't land (for now) in v10.x Sep 23, 2018

targos moved this from Don't land (for now) to Don't land (ever) in v10.x Sep 23, 2018

addaleax removed the dont-land-on-v10.x label Sep 27, 2018

addaleax removed this from Don't land (ever) in v10.x Sep 27, 2018

targos mentioned this pull request Oct 7, 2018

Release proposal: v10.12.0 #23313

Merged

		@@ -892,7 +904,6 @@ def configure_mips(o):
		o['variables']['mips_arch_variant'] = options.mips_arch_variant
		o['variables']['mips_fpu_mode'] = options.mips_fpu_mode

		@@ -955,7 +989,6 @@ def configure_node(o):
		' or newer only.' % (version_checked_str))

		o['variables']['enable_lto'] = b(options.enable_lto)

		@@ -893,6 +905,16 @@ def configure_mips(o):
		o['variables']['mips_fpu_mode'] = options.mips_fpu_mode


		def check_gcc_version(version_checked):

pgo: enabling pgo at configure #21596

pgo: enabling pgo at configure #21596

Conversation

octaviansoldea commented Jun 29, 2018

Checklist

mscdex Jun 30, 2018 • edited

Choose a reason for hiding this comment

octaviansoldea Jun 30, 2018 • edited

Choose a reason for hiding this comment

octaviansoldea commented Jul 10, 2018 • edited

jasnell commented Jul 12, 2018

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

octaviansoldea commented Aug 1, 2018

octaviansoldea commented Aug 2, 2018 • edited

lundibundi 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

lundibundi commented Aug 5, 2018

lundibundi left a comment

Choose a reason for hiding this comment

lundibundi commented Aug 25, 2018

jasnell left a comment

Choose a reason for hiding this comment

octaviansoldea commented Aug 31, 2018

lundibundi commented Sep 2, 2018

addaleax commented Sep 2, 2018

addaleax commented Sep 3, 2018

gabrielschulhof commented Sep 4, 2018

gabrielschulhof commented Sep 4, 2018

misterdjules commented Sep 6, 2018

targos commented Sep 23, 2018

lundibundi commented Sep 25, 2018

addaleax commented Sep 25, 2018

targos commented Sep 25, 2018

octaviansoldea commented Sep 26, 2018

addaleax commented Sep 27, 2018

mscdex Jun 30, 2018 •

edited

octaviansoldea Jun 30, 2018 •

edited

octaviansoldea commented Jul 10, 2018 •

edited

octaviansoldea commented Aug 2, 2018 •

edited