-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
build, win: faster Release rebuilds #17393
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
Conversation
common.gypi
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It adds /LTCG to the linker options, we want to use /LTCG:INCREMENTAL. Right now gyp does not support this option directly, so we add it through additional options.
vcbuild.bat
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's inconsistent with other arguments which looks like <feature> or no<feature>. How about nocctest instead? Or even better - maybe build cctest only if the test argument is specified?
|
Is there any difference in benchmark results? |
|
Updated, changed I did run benchmarks over the weekend, comparing node built with vs2017 - "old" using
They seem to pretty much the same, although I'm not an expert in this. |
ba4ac26 to
9185f89
Compare
Sets Link Time Code Generation to INCREMENTAL improving Release rebuilds speed. Adds no-cctest option to vcbuild.bat, which will skip building cctest.exe PR-URL: nodejs#17393 Reviewed-By: Refael Ackermann <[email protected]>
9185f89 to
ca0bb19
Compare
|
Landed in 8514ea9 |
Sets Link Time Code Generation to INCREMENTAL improving Release rebuilds speed. Adds no-cctest option to vcbuild.bat, which will skip building cctest.exe PR-URL: #17393 Reviewed-By: Refael Ackermann <[email protected]>
Sets Link Time Code Generation to INCREMENTAL improving Release rebuilds speed. Adds no-cctest option to vcbuild.bat, which will skip building cctest.exe PR-URL: #17393 Reviewed-By: Refael Ackermann <[email protected]>
Sets Link Time Code Generation to INCREMENTAL improving Release rebuilds speed. Adds no-cctest option to vcbuild.bat, which will skip building cctest.exe PR-URL: #17393 Reviewed-By: Refael Ackermann <[email protected]>
|
@nodejs/platform-windows is this something we want to backport to 8.x? Seems like windows tooling for 8.x in general has fallen behind |
|
I'd say backport if it lands smoothly. It's mostly a dev time optimization, IMHO the LTS line can take it or leave it... |
Sets Link Time Code Generation to
INCREMENTAL, improving Release rebuilds speed. The/LTCG:INCREMENTALwas added in VS2015 and is not directly supported ingyp, thus it is implemented withAdditionalOptions. Some more information can be found in this vcblog post.Adds
exe-onlyoption tovcbuild.bat, which will build onlynode.exebinary. It will skip buildingcctest, which takes as long as buildingnode.exeitself.Currently, on my box
vcbuildrebuild takes 8 minutes. With those changesvcbuild exe-onlytakes only 1.5 minute.Ref: #17253
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build
/cc @nodejs/build @nodejs/platform-windows