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.

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

pgo: enabling pgo at configure #21596

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
10 participants
@octaviansoldea
Copy link
Contributor

commented Jun 29, 2018

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.

configure Outdated

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

This comment has been minimized.

Copy link
@mscdex

mscdex Jun 30, 2018

Contributor

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()?

This comment has been minimized.

Copy link
@octaviansoldea

octaviansoldea Jun 30, 2018

Author Contributor

Hello

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

@octaviansoldea

@octaviansoldea octaviansoldea force-pushed the octaviansoldea:enabling_pgo branch from fb27526 to 93d6e2f Jul 3, 2018

@octaviansoldea

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

Dear Reviewers

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

@octaviansoldea

@jasnell jasnell requested review from bnoordhuis and addaleax Jul 10, 2018

@jasnell

This comment has been minimized.

@octaviansoldea octaviansoldea force-pushed the octaviansoldea:enabling_pgo branch from d1a8f83 to e515d6e Jul 17, 2018

common.gypi Outdated
],
},],
],},
],

This comment has been minimized.

Copy link
@lundibundi

lundibundi Jul 28, 2018

Member

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

This comment has been minimized.

Copy link
@octaviansoldea

octaviansoldea Aug 2, 2018

Author Contributor

Agree and tried to resolve it.

configure Outdated
@@ -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


This comment has been minimized.

Copy link
@lundibundi

lundibundi Jul 28, 2018

Member

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

This comment has been minimized.

Copy link
@octaviansoldea

octaviansoldea Aug 2, 2018

Author Contributor

Agree and tried to resolve it.

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

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

This comment has been minimized.

Copy link
@lundibundi

lundibundi Jul 28, 2018

Member

Also here, any reason for the removal?

This comment has been minimized.

Copy link
@octaviansoldea

octaviansoldea Aug 2, 2018

Author Contributor

Agree and tried to resolve it.

configure Outdated
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++.")

This comment has been minimized.

Copy link
@lundibundi

lundibundi Jul 28, 2018

Member

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

This comment has been minimized.

Copy link
@octaviansoldea

octaviansoldea Aug 2, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@lundibundi

lundibundi Aug 2, 2018

Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2018

Dear Reviewers

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

@octaviansoldea

@octaviansoldea

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2018

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
Copy link
Member

left a comment

Thanks for your work 👍
Just a few nits.

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


def check_gcc_version(version_checked):

This comment has been minimized.

Copy link
@lundibundi

lundibundi Aug 2, 2018

Member

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

This comment has been minimized.

Copy link
@octaviansoldea

octaviansoldea Aug 3, 2018

Author Contributor

Agree and tried to resolve it.

configure Outdated
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:

This comment has been minimized.

Copy link
@lundibundi

lundibundi Aug 2, 2018

Member

if not check_gcc_version(version_checked):

This comment has been minimized.

Copy link
@octaviansoldea

octaviansoldea Aug 3, 2018

Author Contributor

Agree and tried to resolve it.

configure Outdated
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 '

This comment has been minimized.

Copy link
@lundibundi

lundibundi Aug 2, 2018

Member

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

This comment has been minimized.

Copy link
@octaviansoldea

octaviansoldea Aug 3, 2018

Author Contributor

Agree and tried to resolve it.

configure Outdated
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++.")

This comment has been minimized.

Copy link
@lundibundi

lundibundi Aug 2, 2018

Member

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.

configure Outdated
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:

This comment has been minimized.

Copy link
@lundibundi

lundibundi Aug 2, 2018

Member

Also if not check_gcc_version(version_checked):

This comment has been minimized.

Copy link
@octaviansoldea

octaviansoldea Aug 3, 2018

Author Contributor

Agree and tried to resolve it.

@lundibundi

This comment has been minimized.

Copy link
Member

commented Aug 5, 2018

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

@lundibundi
Copy link
Member

left a comment

LGTM.

@lundibundi

This comment has been minimized.

Copy link
Member

commented Aug 25, 2018

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

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

@jasnell
Copy link
Member

left a comment

still lgtm

@octaviansoldea

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2018

Dear Reviewers

Thank you for your messages. I will answer asap.

@octaviansoldea

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

Refs: #21583
Refs: #1409

@octaviansoldea octaviansoldea force-pushed the octaviansoldea:enabling_pgo branch from 0a16566 to 97967da Aug 31, 2018

@lundibundi

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member

commented Sep 2, 2018

@addaleax

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2018

The latter CI is actually green, so landing.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2018

Landed in 9be1555.

gabrielschulhof added a commit that referenced this pull request Sep 4, 2018

build: enabling pgo at configure
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>

targos added a commit that referenced this pull request Sep 5, 2018

build: enabling pgo at configure
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>

targos added a commit that referenced this pull request Sep 5, 2018

build: enabling pgo at configure
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>

@targos targos referenced this pull request Sep 5, 2018

Merged

Release proposal: v10.10.0 #22716

@misterdjules

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

@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 targos added this to Don't land (for now) in v10.x Sep 23, 2018

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

@targos

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

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

@lundibundi

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

@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

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

@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

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

Should we try to land it again on v10?

@octaviansoldea

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

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 addaleax removed this from Don't land (ever) in v10.x Sep 27, 2018

@addaleax

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

@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.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018

build: enabling pgo at configure
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
PR-URL: nodejs#21596
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>

targos added a commit that referenced this pull request Oct 3, 2018

build: enabling pgo at configure
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>

@targos targos referenced this pull request Oct 7, 2018

Merged

Release proposal: v10.12.0 #23313

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.