Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2087723003:
Use the /PROFILE linker flag for the Windows GN builds. (Closed)
|
Created:
4 years, 6 months ago by Sébastien Marchand Modified:
4 years, 6 months ago CC:
chromium-reviews, Dirk Pranke Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse the /PROFILE linker flag for the Windows GN builds.
Note that we're already using this flag for the GYP builds.
R=chrisha@chromium.org, brucedawson@chromium.org
BUG=621124
Committed: https://crrev.com/3d9805722d2a7fd45ff5d1138c2fb2936f3e9de3
Cr-Commit-Position: refs/heads/master@{#401517}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 12 (2 generated)
PTAL.
+dpranke@ FYI.
https://codereview.chromium.org/2087723003/diff/1/build/config/win/BUILD.gn File build/config/win/BUILD.gn (right): https://codereview.chromium.org/2087723003/diff/1/build/config/win/BUILD.gn#n... build/config/win/BUILD.gn:103: if (!is_debug && !is_win_fastlink && !is_clang) { I didn't notice is_clang checks in the GYP version of this setting, although the PROFILE settings were pretty well spread out so I can't be sure. Is the is_clang check appropriate? Also, do we want /PROFILE on non-syzyasan builds? Also, there has been some discussion of turning on incremental linking for release component builds, and incremental linking is incompatible with /PROFILE. Just checking to make sure we have the right cases. It sounds like generating the FIXUP information is not free so we might as well do it as rarely as possible. I only noticed build breaks from the lack of this on official builds, FWIW.
https://codereview.chromium.org/2087723003/diff/1/build/config/win/BUILD.gn File build/config/win/BUILD.gn (right): https://codereview.chromium.org/2087723003/diff/1/build/config/win/BUILD.gn#n... build/config/win/BUILD.gn:103: if (!is_debug && !is_win_fastlink && !is_clang) { On 2016/06/21 21:28:49, brucedawson wrote: > I didn't notice is_clang checks in the GYP version of this setting, although the > PROFILE settings were pretty well spread out so I can't be sure. Is the is_clang > check appropriate? > > Also, do we want /PROFILE on non-syzyasan builds? > > Also, there has been some discussion of turning on incremental linking for > release component builds, and incremental linking is incompatible with /PROFILE. > > Just checking to make sure we have the right cases. It sounds like generating > the FIXUP information is not free so we might as well do it as rarely as > possible. I only noticed build breaks from the lack of this on official builds, > FWIW. There's no 'is_clang' check in the GYP version of this setting but this is probably a mistake as we don't plan to support the Clang builds (what we're doing with Syzygy doesn't really make sense with Clang). We should keep in mind that we'll need to add an additional check the day we'll turn on incremental linking for the release component builds (there's not so much we can do until then). Generating the FIXUP information only affect the PDB (for a 5% increase in size), and I think that keeping the ability to instrument (with SyzyAsan or for Coverage) 'any' Chrome build without rebuilding is worth this extra cost ?
> Generating the FIXUP information only affect the PDB (for a 5% increase in > size), and I think that keeping the ability to instrument (with SyzyAsan or for > Coverage) 'any' Chrome build without rebuilding is worth this extra cost ? It might be worth measuring the cost of /PROFILE before enabling it where it isn't strictly needed. Windows build times are problematic already...
I ran the tests that I requested in order to see the build-time performance of
/PROFILE. The results are that /PROFILE has a minuscule effect on build times. I
did my tests with a 64-bit component release gn build:
is_component_build = true
is_debug = false
I started by building chrome.dll with -v -d keeprsp. I then added the
depot_tools toolchain to the path:
C:\src\depot_tools\win_toolchain\vs_files\95ddda401ec5678f15eeed01d2bee08fcbc5ee97\win_sdk\bin\setenv
/x64
I then ran this pretty simple batch file (note that deleting the PDB is
important because PDBs normally never get smaller, only larger):
@setlocal
@set target=chrome
@set outdir=out\gn_release_component
@cd /d C:\src\chromium4\src\out\gn_release_component
@del chrome.dll.pdb
@set starttime=%time%
link.exe /nologo /IMPLIB:./chrome.dll.lib /DLL /OUT:./chrome.dll
/PDB:./chrome.dll.pdb /opt:ref /opt:icf /PROFILE @./chrome.dll.rsp
@echo Ran from %starttime% to %time%
@dir chrome.dll* | find /i "chrome.dll"
@del chrome.dll.pdb
@set starttime=%time%
link.exe /nologo /IMPLIB:./chrome.dll.lib /DLL /OUT:./chrome.dll
/PDB:./chrome.dll.pdb /opt:ref /opt:icf @./chrome.dll.rsp
@echo Ran from %starttime% to %time%
@dir chrome.dll* | find /i "chrome.dll"
Typical results are:
C:\src\chromium4\src\out\gn_release_component>link.exe /nologo
/IMPLIB:./chrome.dll.lib /DLL /OUT:./chrome.dll /PDB:./chrome.dll.pdb /opt:ref
/opt:icf /PROFILE @./chrome.dll.rsp
Creating library ./chrome.dll.lib and object ./chrome.dll.exp
Ran from 15:37:07.48 to 15:38:10.64
06/22/2016 03:38 PM 33,193,984 chrome.dll
06/22/2016 03:37 PM 5,519 chrome.dll.exp
06/22/2016 03:37 PM 9,538 chrome.dll.lib
06/22/2016 03:38 PM 731,459,584 chrome.dll.pdb
06/22/2016 03:13 PM 199,125 chrome.dll.rsp
C:\src\chromium4\src\out\gn_release_component>link.exe /nologo
/IMPLIB:./chrome.dll.lib /DLL /OUT:./chrome.dll /PDB:./chrome.dll.pdb /opt:ref
/opt:icf @./chrome.dll.rsp
Creating library ./chrome.dll.lib and object ./chrome.dll.exp
Ran from 15:38:10.76 to 15:39:14.22
06/22/2016 03:39 PM 33,194,496 chrome.dll
06/22/2016 03:38 PM 5,519 chrome.dll.exp
06/22/2016 03:38 PM 9,538 chrome.dll.lib
06/22/2016 03:39 PM 717,451,264 chrome.dll.pdb
06/22/2016 03:13 PM 199,125 chrome.dll.rsp
The exact timing results vary but any slowdown is in the noise. lgtm.
Trivia: /PROFILE implies /opt:ref and /opt:icf so if you don't specify those in
the non-profile case then the DLL, PDB, and build times may all be *worse*.
On 2016/06/22 23:09:16, brucedawson wrote: > I ran the tests that I requested in order to see the build-time performance of > /PROFILE. The results are that /PROFILE has a minuscule effect on build times. I > did my tests with a 64-bit component release gn build: > > is_component_build = true > is_debug = false > > I started by building chrome.dll with -v -d keeprsp. I then added the > depot_tools toolchain to the path: > > > C:\src\depot_tools\win_toolchain\vs_files\95ddda401ec5678f15eeed01d2bee08fcbc5ee97\win_sdk\bin\setenv > /x64 > > I then ran this pretty simple batch file (note that deleting the PDB is > important because PDBs normally never get smaller, only larger): > > > @setlocal > > @set target=chrome > @set outdir=out\gn_release_component > > @cd /d C:\src\chromium4\src\out\gn_release_component > > @del chrome.dll.pdb > @set starttime=%time% > link.exe /nologo /IMPLIB:./chrome.dll.lib /DLL /OUT:./chrome.dll > /PDB:./chrome.dll.pdb /opt:ref /opt:icf /PROFILE @./chrome.dll.rsp > @echo Ran from %starttime% to %time% > @dir chrome.dll* | find /i "chrome.dll" > > @del chrome.dll.pdb > @set starttime=%time% > link.exe /nologo /IMPLIB:./chrome.dll.lib /DLL /OUT:./chrome.dll > /PDB:./chrome.dll.pdb /opt:ref /opt:icf @./chrome.dll.rsp > @echo Ran from %starttime% to %time% > @dir chrome.dll* | find /i "chrome.dll" > > > Typical results are: > > > C:\src\chromium4\src\out\gn_release_component>link.exe /nologo > /IMPLIB:./chrome.dll.lib /DLL /OUT:./chrome.dll /PDB:./chrome.dll.pdb /opt:ref > /opt:icf /PROFILE @./chrome.dll.rsp > Creating library ./chrome.dll.lib and object ./chrome.dll.exp > Ran from 15:37:07.48 to 15:38:10.64 > 06/22/2016 03:38 PM 33,193,984 chrome.dll > 06/22/2016 03:37 PM 5,519 chrome.dll.exp > 06/22/2016 03:37 PM 9,538 chrome.dll.lib > 06/22/2016 03:38 PM 731,459,584 chrome.dll.pdb > 06/22/2016 03:13 PM 199,125 chrome.dll.rsp > > C:\src\chromium4\src\out\gn_release_component>link.exe /nologo > /IMPLIB:./chrome.dll.lib /DLL /OUT:./chrome.dll /PDB:./chrome.dll.pdb /opt:ref > /opt:icf @./chrome.dll.rsp > Creating library ./chrome.dll.lib and object ./chrome.dll.exp > Ran from 15:38:10.76 to 15:39:14.22 > 06/22/2016 03:39 PM 33,194,496 chrome.dll > 06/22/2016 03:38 PM 5,519 chrome.dll.exp > 06/22/2016 03:38 PM 9,538 chrome.dll.lib > 06/22/2016 03:39 PM 717,451,264 chrome.dll.pdb > 06/22/2016 03:13 PM 199,125 chrome.dll.rsp > > > The exact timing results vary but any slowdown is in the noise. lgtm. > > Trivia: /PROFILE implies /opt:ref and /opt:icf so if you don't specify those in > the non-profile case then the DLL, PDB, and build times may all be *worse*. Wow, thanks for doing my job :), I wanted to take care of this today but didn't had much time (/free cpu cores) for this.
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2087723003/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Use the /PROFILE linker flag for the Windows GN builds. Note that we're already using this flag for the GYP builds. R=chrisha@chromium.org, brucedawson@chromium.org BUG=621124 ========== to ========== Use the /PROFILE linker flag for the Windows GN builds. Note that we're already using this flag for the GYP builds. R=chrisha@chromium.org, brucedawson@chromium.org BUG=621124 Committed: https://crrev.com/3d9805722d2a7fd45ff5d1138c2fb2936f3e9de3 Cr-Commit-Position: refs/heads/master@{#401517} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/3d9805722d2a7fd45ff5d1138c2fb2936f3e9de3 Cr-Commit-Position: refs/heads/master@{#401517} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
