Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7464040:
Integrate syzygy (optionally) into the chrome build process. (Closed)
|
Created:
9 years, 5 months ago by Roger McFarlane (Google) Modified:
9 years, 4 months ago Reviewers:
Sigurður Ásgeirsson, jam, chrisha, M-A Ruel, Jeff Bailey (chromium), Nicolas Sylvain, commit-bot: I haz the power CC:
chromium-reviews, mmenke Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionIntegrate syzygy (optionally) into the chrome build process.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95620
Patch Set 1 : Initial review. #
Total comments: 4
Patch Set 2 : fixed dependencies #
Total comments: 4
Patch Set 3 : Address nits. #
Total comments: 2
Patch Set 4 : always use intermediate chome_dll target on windows and pull syzygy from public repo #
Total comments: 14
Patch Set 5 : fix a typo and use built-in 'copies' target action #Patch Set 6 : renamed syzygy.gpyi to chrome_dll_syzygy.gypi and pinned syzygy release revision #Patch Set 7 : build correctly when fastbuild=1 is defined in the GYP environment #Patch Set 8 : don't turn on /PROFILE when fastbuild==1 #Patch Set 9 : gcl sync to latest for an additional try-server invocation #
Messages
Total messages: 28 (0 generated)
PTAL
lgtm http://codereview.chromium.org/7464040/diff/2001/chrome/chrome_dll.gypi File chrome/chrome_dll.gypi (right): http://codereview.chromium.org/7464040/diff/2001/chrome/chrome_dll.gypi#newco... chrome/chrome_dll.gypi:139: 'Profile': 'true', Wasn't this already addressed somewhere else? ie: in base.gyp, or common.gypi? http://codereview.chromium.org/7464040/diff/2001/chrome/installer/mini_instal... File chrome/installer/mini_installer.gyp (right): http://codereview.chromium.org/7464040/diff/2001/chrome/installer/mini_instal... chrome/installer/mini_installer.gyp:9: 'optimize_with_syzygy%': 0, What does the trailing '%' do?
http://codereview.chromium.org/7464040/diff/2001/chrome/chrome_dll.gypi File chrome/chrome_dll.gypi (right): http://codereview.chromium.org/7464040/diff/2001/chrome/chrome_dll.gypi#newco... chrome/chrome_dll.gypi:137: # This corresponse to the /PROFILE flag which ensures corresponse -> corresponds? http://codereview.chromium.org/7464040/diff/2001/chrome/syzygy.gypi File chrome/syzygy.gypi (right): http://codereview.chromium.org/7464040/diff/2001/chrome/syzygy.gypi#newcode42 chrome/syzygy.gypi:42: '--replace-originals', Ah. This is going to be flaky. Let's discuss...
The profile flag is already enabled for all official builds, see http://src.chromium.org/viewvc/chrome/trunk/src/build/internal/release_impl_o.... There's no harm in doing this also under the syzygy flag. Modifying already built binaries in the output directory is not a good solution, and will be flaky. Since the modified files are not in the action outputs, there's no reason why the build shouldn't go ahead and suck in the unmodified files into the RES file that goes into the mini-installer before they've been mutated, plus there will be flakiness with non-clobber builds. On Tue, Jul 26, 2011 at 11:47 PM, <chrisha@chromium.org> wrote: > lgtm > > > http://codereview.chromium.**org/7464040/diff/2001/chrome/** > chrome_dll.gypi<http://codereview.chromium.org/7464040/diff/2001/chrome/chrome_dll.gypi> > File chrome/chrome_dll.gypi (right): > > http://codereview.chromium.**org/7464040/diff/2001/chrome/** > chrome_dll.gypi#newcode139<http://codereview.chromium.org/7464040/diff/2001/chrome/chrome_dll.gypi#newcode139> > chrome/chrome_dll.gypi:139: 'Profile': 'true', > Wasn't this already addressed somewhere else? ie: in base.gyp, or > common.gypi? > > http://codereview.chromium.**org/7464040/diff/2001/chrome/** > installer/mini_installer.gyp<http://codereview.chromium.org/7464040/diff/2001/chrome/installer/mini_installer.gyp> > File chrome/installer/mini_**installer.gyp (right): > > http://codereview.chromium.**org/7464040/diff/2001/chrome/** > installer/mini_installer.gyp#**newcode9<http://codereview.chromium.org/7464040/diff/2001/chrome/installer/mini_installer.gyp#newcode9> > chrome/installer/mini_**installer.gyp:9: 'optimize_with_syzygy%': 0, > What does the trailing '%' do? > > > http://codereview.chromium.**org/7464040/<http://codereview.chromium.org/7464... >
PTAL. Fixed build dependency by turning (on optimize_with_syzygy==1) the original chrome_dll targert into an intermediate with a different name, and replacing it with the optimization step.
lgtm http://codereview.chromium.org/7464040/diff/21001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): http://codereview.chromium.org/7464040/diff/21001/chrome/chrome_exe.gypi#newc... chrome/chrome_exe.gypi:422: # original chrome_dll, so that the optimize step can run run a run run? http://codereview.chromium.org/7464040/diff/21001/chrome/syzygy.gypi File chrome/syzygy.gypi (right): http://codereview.chromium.org/7464040/diff/21001/chrome/syzygy.gypi#newcode19 chrome/syzygy.gypi:19: #'default_extensions', Why are these commented out?
Thanks. I think I need the blessing of the build gods. Jeff/Marc? http://codereview.chromium.org/7464040/diff/21001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): http://codereview.chromium.org/7464040/diff/21001/chrome/chrome_exe.gypi#newc... chrome/chrome_exe.gypi:422: # original chrome_dll, so that the optimize step can run run a On 2011/08/02 13:31:44, chrisha wrote: > run run? Fixed. http://codereview.chromium.org/7464040/diff/21001/chrome/syzygy.gypi File chrome/syzygy.gypi (right): http://codereview.chromium.org/7464040/diff/21001/chrome/syzygy.gypi#newcode19 chrome/syzygy.gypi:19: #'default_extensions', On 2011/08/02 13:31:44, chrisha wrote: > Why are these commented out? Was playing with dependencies ... meant to delete those lines before committing. Done.
or course, that should have read: Jeff / Marc-Antoine? Sorry for butchering your name Marc-Antoine.
I'm on the fence here. While the conditional target name is clever, I think it complexifies it more than warranted. I prefer to have the flow unconditional, e.g. always have the original_chrome_dll and have the 'optimization' step to be conditionally one of: - Just hardlink - Run syzygy wdyt? http://codereview.chromium.org/7464040/diff/21002/chrome/chrome_dll.gypi File chrome/chrome_dll.gypi (right): http://codereview.chromium.org/7464040/diff/21002/chrome/chrome_dll.gypi#newc... chrome/chrome_dll.gypi:66: 'variables': { I think it warrants a small comment explaining why you are doing this. I know why but it may not be clear for anyone else.
PTAL Updated so that (on Windows) it always has the 2-step process for generating the chrome dll and pdb files. If syzygy is enabled then the second step is the optimization process, otherwise, it's just a copy from the initial directory to the product directory. I changed the name to chrome_dll_initial, btw ... so it shows up more or less where developers would expect to see it in the solution. http://codereview.chromium.org/7464040/diff/21002/chrome/chrome_dll.gypi File chrome/chrome_dll.gypi (right): http://codereview.chromium.org/7464040/diff/21002/chrome/chrome_dll.gypi#newc... chrome/chrome_dll.gypi:66: 'variables': { On 2011/08/02 15:25:14, Marc-Antoine Ruel wrote: > I think it warrants a small comment explaining why you are doing this. I know > why but it may not be clear for anyone else. Done. http://codereview.chromium.org/7464040/diff/28004/DEPS File DEPS (right): http://codereview.chromium.org/7464040/diff/28004/DEPS#newcode317 DEPS:317: "https://sawbuck.googlecode.com/svn/trunk/syzygy/binaries", Pull this from our repo or commit our releases directly into the chrome tree? i.e.: /trunk/deps/third_party/syzygy/binaries
http://codereview.chromium.org/7464040/diff/28004/DEPS File DEPS (right): http://codereview.chromium.org/7464040/diff/28004/DEPS#newcode317 DEPS:317: "https://sawbuck.googlecode.com/svn/trunk/syzygy/binaries", On 2011/08/03 16:22:28, Roger McFarlane (Google) wrote: > Pull this from our repo or commit our releases directly into the chrome tree? > i.e.: > > /trunk/deps/third_party/syzygy/binaries nsylvain@ further suggests that everything should be pinned in thest DEPS files (actually he was talking about the internal DEPS at the time, but I would presume it applies here as well).
A couple of comments... http://codereview.chromium.org/7464040/diff/28004/chrome/chrome_dll.gypi File chrome/chrome_dll.gypi (right): http://codereview.chromium.org/7464040/diff/28004/chrome/chrome_dll.gypi#newc... chrome/chrome_dll.gypi:151: 'ProgramDatabaseFile': '$(OutDir)\\initial\\chrome_dll.pdb', Do you want this to be chrome_dll_initial.pdb as well? http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi File chrome/syzygy.gypi (right): http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi#newcode60 chrome/syzygy.gypi:60: 'action': ['copy', '<(input)', '<(output)'], There's a 'copies' functionality built in to GYP. Check out http://codesearch.google.com/#OAMlx_jo-ck/o3d/build/libs.gyp&exact_package=ch... for an example.
http://codereview.chromium.org/7464040/diff/28004/DEPS File DEPS (right): http://codereview.chromium.org/7464040/diff/28004/DEPS#newcode317 DEPS:317: "https://sawbuck.googlecode.com/svn/trunk/syzygy/binaries", On 2011/08/03 17:06:55, Roger McFarlane (Google) wrote: > nsylvain@ further suggests that everything should be pinned in thest DEPS files Yes please. http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi File chrome/syzygy.gypi (right): http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi#newcode12 chrome/syzygy.gypi:12: 'target_name': 'chrome_dll', I still don't like having this target in a separate file. It makes maintenance significantly more confusing. What is the goal of putting this in a separate file? http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi#newcode60 chrome/syzygy.gypi:60: 'action': ['copy', '<(input)', '<(output)'], On 2011/08/03 17:16:27, chrisha wrote: > There's a 'copies' functionality built in to GYP. Check out > http://codesearch.google.com/#OAMlx_jo-ck/o3d/build/libs.gyp&exact_package=ch... > for an example. hardlink ftw?
http://codereview.chromium.org/7464040/diff/28004/chrome/chrome_dll.gypi File chrome/chrome_dll.gypi (right): http://codereview.chromium.org/7464040/diff/28004/chrome/chrome_dll.gypi#newc... chrome/chrome_dll.gypi:151: 'ProgramDatabaseFile': '$(OutDir)\\initial\\chrome_dll.pdb', On 2011/08/03 17:16:27, chrisha wrote: > Do you want this to be chrome_dll_initial.pdb as well? No, I retained the original basenames for the files (chrome.dll and chrome_dll.pdb) so that they would still match the expectations of all downstream consumers. The names of these things get baked into their dependents (i.e., chrome_dll.lib knows to look for chrome.dll, which knows its pdb file is chrome_dll.pdb). http://codereview.chromium.org/7464040/diff/28004/chrome/chrome_dll.gypi#newc... chrome/chrome_dll.gypi:152: 'OutputFile': '$(OutDir)\\initial\chrome.dll', Oops, that should be a double backslash between "initial" and "chrome.dll". Python preserves the backslash for unrecognized escape sequences, so it works. Will fix anyway.
http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi File chrome/syzygy.gypi (right): http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi#newcode12 chrome/syzygy.gypi:12: 'target_name': 'chrome_dll', On 2011/08/03 17:32:59, Marc-Antoine Ruel wrote: > I still don't like having this target in a separate file. It makes maintenance > significantly more confusing. What is the goal of putting this in a separate > file? Merging this into chrome_dll.gypi would introduce a circular depencency in the includes. The optimization step involves launching chrome and profiling its startup. So, it depends on chrome and on the initial chrome_dll. chrome also depends on the initial chrome_dll. http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi#newcode60 chrome/syzygy.gypi:60: 'action': ['copy', '<(input)', '<(output)'], On 2011/08/03 17:16:27, chrisha wrote: > There's a 'copies' functionality built in to GYP. Check out > http://codesearch.google.com/#OAMlx_jo-ck/o3d/build/libs.gyp&exact_package=ch... > for an example. Excellent! Thanks for the tip. http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi#newcode60 chrome/syzygy.gypi:60: 'action': ['copy', '<(input)', '<(output)'], On 2011/08/03 17:32:59, Marc-Antoine Ruel wrote: > On 2011/08/03 17:16:27, chrisha wrote: > > There's a 'copies' functionality built in to GYP. Check out > > > http://codesearch.google.com/#OAMlx_jo-ck/o3d/build/libs.gyp&exact_package=ch... > > for an example. > > hardlink ftw? Please elucidate? :) Not sure a hard link would be ideal if we want to support changing an existing dev environment between modes (i.e., developer sets GYP_DEFINES=optimize_with_syzygy=1 and reruns the hooks).
On Wed, Aug 3, 2011 at 1:56 PM, <rogerm@google.com> wrote: > > http://codereview.chromium.**org/7464040/diff/28004/chrome/**syzygy.gypi<http... > File chrome/syzygy.gypi (right): > > http://codereview.chromium.**org/7464040/diff/28004/chrome/** > syzygy.gypi#newcode12<http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi#newcode12> > chrome/syzygy.gypi:12: 'target_name': 'chrome_dll', > On 2011/08/03 17:32:59, Marc-Antoine Ruel wrote: > >> I still don't like having this target in a separate file. It makes >> > maintenance > >> significantly more confusing. What is the goal of putting this in a >> > separate > >> file? >> > > Merging this into chrome_dll.gypi would introduce a circular depencency > in the includes. The optimization step involves launching chrome and > profiling its startup. So, it depends on chrome and on the initial > chrome_dll. chrome also depends on the initial chrome_dll. > > > http://codereview.chromium.**org/7464040/diff/28004/chrome/** > syzygy.gypi#newcode60<http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi#newcode60> > chrome/syzygy.gypi:60: 'action': ['copy', '<(input)', '<(output)'], > On 2011/08/03 17:16:27, chrisha wrote: > >> There's a 'copies' functionality built in to GYP. Check out >> > > http://codesearch.google.com/#**OAMlx_jo-ck/o3d/build/libs.** > gyp&exact_package=chromium&q=%**2522%27type%27:%2520%27none%** > 27%2522%2520file:gyp%24%**2520copy&type=cs&l=348<http://codesearch.google.com/#OAMlx_jo-ck/o3d/build/libs.gyp&exact_package=chromium&q=%2522%27type%27:%2520%27none%27%2522%2520file:gyp%24%2520copy&type=cs&l=348> > >> for an example. >> > > Excellent! Thanks for the tip. > > > http://codereview.chromium.**org/7464040/diff/28004/chrome/** > syzygy.gypi#newcode60<http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi#newcode60> > chrome/syzygy.gypi:60: 'action': ['copy', '<(input)', '<(output)'], > On 2011/08/03 17:32:59, Marc-Antoine Ruel wrote: > >> On 2011/08/03 17:16:27, chrisha wrote: >> > There's a 'copies' functionality built in to GYP. Check out >> > >> > > http://codesearch.google.com/#**OAMlx_jo-ck/o3d/build/libs.** > gyp&exact_package=chromium&q=%**252522%2527type%2527:%252520%** > 2527none%2527%252522%**252520file:gyp%2524%**252520copy&type=cs&l=348<http://codesearch.google.com/#OAMlx_jo-ck/o3d/build/libs.gyp&exact_package=chromium&q=%252522%2527type%2527:%252520%2527none%2527%252522%252520file:gyp%2524%252520copy&type=cs&l=348> > > > for an example. >> > > hardlink ftw? >> > > I'm wary of hardlinks on Windows, is there anything wrong with a copy? Please elucidate? > > :) > > Not sure a hard link would be ideal if we want to support changing an > existing dev environment between modes (i.e., developer sets > GYP_DEFINES=optimize_with_**syzygy=1 and reruns the hooks). > > > http://codereview.chromium.**org/7464040/<http://codereview.chromium.org/7464... >
So, what's the verdict? http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi File chrome/syzygy.gypi (right): http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi#newcode60 chrome/syzygy.gypi:60: 'action': ['copy', '<(input)', '<(output)'], On 2011/08/03 17:56:35, Roger McFarlane (Google) wrote: > On 2011/08/03 17:16:27, chrisha wrote: > > There's a 'copies' functionality built in to GYP. Check out > > > http://codesearch.google.com/#OAMlx_jo-ck/o3d/build/libs.gyp&exact_package=ch... > > for an example. > > Excellent! Thanks for the tip. Done. http://codereview.chromium.org/7464040/diff/28004/chrome/syzygy.gypi#newcode60 chrome/syzygy.gypi:60: 'action': ['copy', '<(input)', '<(output)'], On 2011/08/03 17:56:35, Roger McFarlane (Google) wrote: > On 2011/08/03 17:32:59, Marc-Antoine Ruel wrote: > > On 2011/08/03 17:16:27, chrisha wrote: > > > There's a 'copies' functionality built in to GYP. Check out > > > > > > http://codesearch.google.com/#OAMlx_jo-ck/o3d/build/libs.gyp&exact_package=ch... > > > for an example. > > > > hardlink ftw? > > Please elucidate? > > :) > > Not sure a hard link would be ideal if we want to support changing an existing > dev environment between modes (i.e., developer sets > GYP_DEFINES=optimize_with_syzygy=1 and reruns the hooks). Siggi's reply seems to have vanished into the ether ... siggi: I'm wary of hardlinks on Windows, is there anything wrong with a copy?
I don't follow your point of circular loop as a reason to keep this definition in a separate .gypi file.
On 2011/08/03 18:37:14, Marc-Antoine Ruel wrote:
> I don't follow your point of circular loop as a reason to keep this definition
> in a separate .gypi file.
My understanding of your suggestion would be that the definition of the new
chrome_dll target would belong in chrome_dll.gypi, right?
So, the problem with putting it there would be the following dependency graph,
unless there's a way to forward declare targets that I'm not aware of.
'include': [
...,
'chrome_dll.gypi', # defines chrome_dll, chrome_dll_initial, depends on
chrome
'chrome_exe.gypi', # defines chrome, depends on chrome_dll_initial
...,
],
You get an error from gyp_chromium when it tries to look up chrome in the
dependencies for chrome_dll before chrome_exe.gypi has been included.
I could, however, move the definiton of chrome_dll to chrome_exe.gypi ... but it
doesn't really belong there either.
I could rename chrome_dll.gypi to chrome_dll_initial.gypi ... but that would
surprise people more, I thought, then introducing a new file ... the name of
which, I'm happy to change, btw.
My understanding of your suggestion would be that the definition of the new
chrome_dll target would belong in chrome_dll.gypi, right?
So, the problem with putting it there would be the following dependency
graph, unless there's a way to forward declare targets that I'm not aware
of.
'include': [
...,
'chrome_dll.gypi', # defines chrome_dll, chrome_dll_initial, depends
on chrome
'chrome_exe.gypi', # defines chrome, depends on chrome_dll_initial
...,
],
You get an error from gyp_chromium when it tries to look up chrome in the
dependencies for chrome_dll before chrome_exe.gypi has been included.
I could, however, move the definiton of chrome_dll to chrome_exe.gypi ...
but it doesn't really belong there either.
I could rename chrome_dll.gypi to chrome_dll_initial.gypi ... but that would
surprise people more, I thought, then introducing a new file ... the name of
which, I'm happy to change, btw.
Roger
On 3 August 2011 14:27, <maruel@chromium.org> wrote:
> I don't follow your point of circular loop as a reason to keep this
> definition
> in a separate .gypi file.
>
>
http://codereview.chromium.**org/7464040/<http://codereview.chromium.org/7464...
>
synchronized with maruel@ via IM/VC.
lgtm
Ok, hopefully the last iteration on this CL. Renamed the new gypi to chrome_dll_syzygy.gypi and added a few comments per the suggestion of maruel@. Also, pinned the syzygy release revision. chrisha@, do you mind pulling a patch to your machine and confirming that it does indeed work on someone elses box before I commit?
So, it seems that the windows try servers suppress the generation of PDB files; the copy step is failing because the PDB doesn't exist (with syzygy disabled). Will adjust the actions to handle this case. This implies no syzygy enabled try server builds, which we weren't looking for anyway. On Aug 3, 2011 4:07 PM, <rogerm@google.com> wrote: > Ok, hopefully the last iteration on this CL. > > Renamed the new gypi to chrome_dll_syzygy.gypi and added a few comments per > the > suggestion of maruel@. Also, pinned the syzygy release revision. > > chrisha@, do you mind pulling a patch to your machine and confirming that it > does indeed work on someone elses box before I commit? > > http://codereview.chromium.org/7464040/
committed.
Change committed as 95620
This now makes building 'chrome' not build chrome_dll, which seems like a regression. i.e. now as a developer, after syncing I have to select both targets to build. Is this intended?
I've gotten a couple of comments on the non-obviousness of the new flow. See: http://codereview.chromium.org/7598008 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
