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.

Sign up for GitHub

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

Jump to bottom

feat: enable whole-program optimization native modules by default #36937

Merged
merged 3 commits into from
Jan 25, 2023
Merged

feat: enable whole-program optimization native modules by default #36937

merged 3 commits into from
Jan 25, 2023

Conversation

kyrylo-hrechykhin
Copy link
Contributor

Description of Change

Enable whole-program optimization by default in script that generates config.gypi needed for electron native modules.
Related work item: #36936

Checklist

Release Notes

Notes: Whole-program optimization is enabled by default in electron node headers config file.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jan 18, 2023
@kyrylo-hrechykhin
Copy link
Contributor Author

@zcbenz @miniak , please review the change.

zcbenz
zcbenz reviewed Jan 19, 2023
View reviewed changes
Copy link
Member

@zcbenz zcbenz left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node's upstream headers do not have this flag, but I'm good adding the flag since it is actually enabled in Electron's build.

@zcbenz zcbenz added semver/patch backwards-compatible bug fixes no-backport labels Jan 19, 2023
@kyrylo-hrechykhin kyrylo-hrechykhin force-pushed the 36936-enable-whole-program-optimization-in-config-gypi branch from 3d8e07f to 7ca6017 Compare January 19, 2023 11:45
@kyrylo-hrechykhin
Copy link
Contributor Author

The change breaks linux build as LTCG is only supported on Windows. Fixing it.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jan 19, 2023
@kyrylo-hrechykhin
Copy link
Contributor Author

Build is failing. Investigating.

@kyrylo-hrechykhin kyrylo-hrechykhin force-pushed the 36936-enable-whole-program-optimization-in-config-gypi branch from 9f93b45 to a32e3ab Compare January 23, 2023 17:30
@kyrylo-hrechykhin kyrylo-hrechykhin force-pushed the 36936-enable-whole-program-optimization-in-config-gypi branch from a32e3ab to 78cca97 Compare January 24, 2023 15:10
@kyrylo-hrechykhin
Copy link
Contributor Author

The build failures were not related to the changes. Changes are ready for merge.
@zcbenz

@MarshallOfSound MarshallOfSound merged commit a59f11f into electron:main Jan 25, 2023
@release-clerk
Copy link

release-clerk bot commented Jan 25, 2023

Release Notes Persisted

Whole-program optimization is enabled by default in electron node headers config file.

@deepak1556
Copy link
Member

deepak1556 commented Jan 26, 2023 •
edited

@kyrylo-hrechykhin @zcbenz why do we need to specify this explicitly when the default for addons is to build with ltcg https://github.com/nodejs/node-gyp/blob/main/addon.gypi#L5 ? Does this flag get overriden during the configure stage ?

@zcbenz
Copy link
Member

zcbenz commented Jan 26, 2023

The common.gypi in node's headers tarball also has the variable defined, I'm not sure which one takes precedence though, .

@kyrylo-hrechykhin
Copy link
Contributor Author

@deepak1556 , @zcbenz , I do not know on which stage this variable changes. But it is clear that config.gypi generated has optimization disabled.

@deepak1556
Copy link
Member

deepak1556 commented Jan 26, 2023 •
edited

Given that both common.gypi and addon.gypi have different default values, seems like a good change to have the value set via our generated config.gypi which will override the defaults.

@kyrylo-hrechykhin kyrylo-hrechykhin deleted the 36936-enable-whole-program-optimization-in-config-gypi branch January 26, 2023 12:52
@kyrylo-hrechykhin
Copy link
Contributor Author

@deepak1556 , I also believe this change may be beneficial for users who consume electron release branches. Is it possible to backport this change to release branches as well?

@deepak1556
Copy link
Member

@zcbenz added the original no-backport label, @zcbenz are you good with backporting this to supported release branches ? This seems more like a bug fix to me.

@zcbenz
Copy link
Member

zcbenz commented Jan 27, 2023

I'm good backporting this 👍

@zcbenz zcbenz added target/21-x-y PR should also be added to the "21-x-y" branch. and removed no-backport labels Jan 27, 2023
@zcbenz zcbenz added target/22-x-y PR should also be added to the "22-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. labels Jan 27, 2023
@zcbenz
Copy link
Member

zcbenz commented Jan 27, 2023

/trop run backport

@trop
Copy link
Contributor

trop bot commented Jan 27, 2023

The backport process for this PR has been manually initiated - here we go! :D

This was referenced Jan 27, 2023
Merged
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Jan 27, 2023

I have automatically backported this PR to "23-x-y", please check out #37046

@trop trop bot added in-flight/23-x-y and removed target/23-x-y PR should also be added to the "23-x-y" branch. labels Jan 27, 2023
@trop
Copy link
Contributor

trop bot commented Jan 27, 2023

I have automatically backported this PR to "22-x-y", please check out #37047

@trop
Copy link
Contributor

trop bot commented Jan 27, 2023

I have automatically backported this PR to "21-x-y", please check out #37048

@trop trop bot added in-flight/22-x-y merged/21-x-y PR was merged to the "21-x-y" branch. merged/23-x-y PR was merged to the "23-x-y" branch. merged/22-x-y PR was merged to the "22-x-y" branch. and removed target/22-x-y PR should also be added to the "22-x-y" branch. target/21-x-y PR should also be added to the "21-x-y" branch. in-flight/21-x-y labels Jan 27, 2023
@Soltus Soltus mentioned this pull request Feb 16, 2023
Closed
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@kyrylo-hrechykhin @khalwa
feat: enable whole-program optimization native modules by default (el…
1e27639 
…ectron#36937)

* feat: enable whole-program optimization

Enable whole-program optimization in electron native modules by default.

* pass --with-ltcg to configure.py instead of setting variable

* enable ltcg only on windows

Co-authored-by: Kyrylo Hrechykhin <khrechykhin@microsoft.com>
gecko19 pushed a commit to brightsign/electron that referenced this pull request Feb 28, 2023
@kyrylo-hrechykhin @gecko19
feat: enable whole-program optimization native modules by default (el…
981a6c6 
…ectron#36937)

* feat: enable whole-program optimization

Enable whole-program optimization in electron native modules by default.

* pass --with-ltcg to configure.py instead of setting variable

* enable ltcg only on windows

Co-authored-by: Kyrylo Hrechykhin <khrechykhin@microsoft.com>
@commit-lint commit-lint bot mentioned this pull request Apr 16, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codebytere codebytere codebytere approved these changes

@zcbenz zcbenz Awaiting requested review from zcbenz

Assignees
No one assigned
Labels
merged/21-x-y PR was merged to the "21-x-y" branch. merged/22-x-y PR was merged to the "22-x-y" branch. merged/23-x-y PR was merged to the "23-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kyrylo-hrechykhin @deepak1556 @zcbenz @codebytere @MarshallOfSound