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

build, deps, test, tools: avoid excessive LTO, debug information #49063

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rockdrilla
Copy link
Contributor

@rockdrilla rockdrilla commented Aug 8, 2023

Signed-off-by: Konstantin Demin rockdrilla@gmail.com

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. dependencies Pull requests that update a dependency file. i18n-api Issues and PRs related to the i18n implementation. icu Issues and PRs related to the ICU dependency. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Aug 8, 2023
@rockdrilla rockdrilla force-pushed the build-flags branch 2 times, most recently from 46e78e0 to 8f1e1e3 Compare August 8, 2023 06:52
@rockdrilla
Copy link
Contributor Author

Maybe I should replace this:

  # Avoid excessive PGO
  ['enable_pgo_generate=="true"', {
    'cflags!': [ '-fprofile-generate' ],
    'ldflags!': [ '-fprofile-generate' ],
  }, ],
  ['enable_pgo_use=="true"', {
    'cflags!': [ '-fprofile-use -fprofile-correction' ],
    'ldflags!': [ '-fprofile-use -fprofile-correction' ],
  }, ],

with that?

  # Avoid excessive PGO
  ['enable_pgo_generate=="true"', {
    'cflags!': [ '<(pgo_generate)' ],
    'ldflags!': [ '<(pgo_generate)' ],
  }, ],
  ['enable_pgo_use=="true"', {
    'cflags!': [ '<(pgo_use)' ],
    'ldflags!': [ '<(pgo_use)' ],
  }, ],

@rockdrilla
Copy link
Contributor Author

One more catch: replace this

  # Avoid excessive LTO
  ['enable_lto=="true"', {
    'ldflags': [ '-fno-lto' ],
  }],

with that?

  # Avoid excessive LTO
  ['enable_lto=="true"', {
    'cflags': [ '-fno-lto' ],
    'cflags!': [ '<(lto)' ],
    'ldflags': [ '-fno-lto' ],
    'ldflags!': [ '<(lto)' ],
  }],

@rockdrilla rockdrilla marked this pull request as draft August 8, 2023 09:55
@rockdrilla rockdrilla changed the title build, deps, tools: avoid excessive LTO, PGO, debug information build, deps, test, tools: avoid excessive LTO, debug information Sep 16, 2023
@rockdrilla
Copy link
Contributor Author

I'd perform more detailed excessive PGO exclusion later (still in progress).

@rockdrilla rockdrilla marked this pull request as ready for review September 16, 2023 19:49
Don't link intermediate executables with LTO in order to speed up
overall build time.

Signed-off-by: Konstantin Demin <rockdrilla@gmail.com>
condition doesn't apply to this target type (introduced in c3cd75d)

Signed-off-by: Konstantin Demin <rockdrilla@gmail.com>
Don't build intermediate executables with debug information in order
to save some disk space during build.

Signed-off-by: Konstantin Demin <rockdrilla@gmail.com>
@rockdrilla
Copy link
Contributor Author

friendly ping @bnoordhuis, @jasnell, @mhdawson, @richardlau
also PTAL at linked MRs for v18.x and v20.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. dependencies Pull requests that update a dependency file. i18n-api Issues and PRs related to the i18n implementation. icu Issues and PRs related to the ICU dependency. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants