Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2968963002:
Some documentation for the jumbo feature. (Closed)
|
Created:
3 years, 5 months ago by Daniel Bratell Modified:
3 years, 5 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSome documentation for the jumbo feature.
R=dpranke@chromium.org
BUG=713317
Review-Url: https://codereview.chromium.org/2968963002
Cr-Commit-Position: refs/heads/master@{#485222}
Committed: https://chromium.googlesource.com/chromium/src/+/dbfab9c2f00000115eb304de54a7b2ed0a4663ac
Patch Set 1 #
Total comments: 13
Patch Set 2 : Addressed review comments #Patch Set 3 : Now with line breaks #Patch Set 4 : Rebased #Messages
Total messages: 24 (13 generated)
bratell@opera.com changed reviewers: + brucedawson@chromium.org
Something like this?
Thanks for writing this up. I believe the 80-character limit is supposed to apply to .md files as well, and it certainly makes reviewing easier. https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md File docs/jumbo.md (right): https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md#newcode3 docs/jumbo.md:3: To improve compilation times it is possible to use "unity builds", called Jumbo builds, in Chromium. The idea is to merge many compilation units and compile them together. Since a large portion of Chromium's code is in shared header files that dramatically reduces the total amount of work needed. compilation units-> translation units. https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md#newcode11 docs/jumbo.md:11: Jumbo is currently implemented as a combined `gn` template and a python script. Eventually it may become at native `gn` feature. By using the template `jumbo_target`, each target will split into one action to "merge" the files and one action to compile the merged files and any files left outside the merge. 'at native' -> 'a native' https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md#newcode17 docs/jumbo.md:17: The "merge" is currently done by creating wrapper files that include the source files. "include" or "#include" - there's a big difference. https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md#newcode23 docs/jumbo.md:23: * Everything compiles faster. When fully enabled everywhere this can **save hours** for a single full compile on a moderate computer. -< for a single full compile on a moderate computer. -> for a full build on a moderate computer. It doesn't make sense to call it a full 'compile' because it's much more than just compiling. BTW, my new four-core *laptop* built Chrome in 2:04:30. Jumbo builds would have helped, I'm sure, but I'm curious as to what computers take long enough to build Chrome that jumbo builds could possibly save hours. https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md#newcode24 docs/jumbo.md:24: * Linking is faster because there is less debug data to merge. -< less debug data to merge. -> less redundant data (debug information, inline functions) to merge. Many inline functions, especially those that are exported, are generated in every translation unit and this also needs to be deduped by the linker. https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md#newcode29 docs/jumbo.md:29: * By merging many files, symbols in different `cc` files can collide and cause compilation errors. -< symbols in different `cc` files can collide and cause compilation errors. -> symbols that have internal linkage in different `cc` files can collide and cause compilation errors.
Not sure about the long lines. I have used http://dillinger.io to preview the markdown and it inserted line breaks whenever the source had a line breaks so to see it rendered properly I made the lines long. https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md File docs/jumbo.md (right): https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md#newcode23 docs/jumbo.md:23: * Everything compiles faster. When fully enabled everywhere this can **save hours** for a single full compile on a moderate computer. On 2017/07/05 21:50:41, brucedawson wrote: > -< for a single full compile on a moderate computer. > -> for a full build on a moderate computer. > > It doesn't make sense to call it a full 'compile' because it's much more than > just compiling. > > BTW, my new four-core *laptop* built Chrome in 2:04:30. Jumbo builds would have > helped, I'm sure, but I'm curious as to what computers take long enough to build > Chrome that jumbo builds could possibly save hours. I think the answer is "tests". Browser tests and unit tests take a long time to compile and link. When I've test build jumbo recently I've included the target blink_tests so I got at least some tests built. Looked at the hardware for the slowest recent Windows build. It was on a build slave using a Intel(R) Xeon(R) CPU E3-1241 v3 @ 3.50GHz. Probably quite similar to a normal desktop i7 CPU.
Uploaded a new version. Kept the long lines so I could use the dillinger.io preview. Should this be linked from somewhere as well? https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md File docs/jumbo.md (right): https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md#newcode3 docs/jumbo.md:3: To improve compilation times it is possible to use "unity builds", called Jumbo builds, in Chromium. The idea is to merge many compilation units and compile them together. Since a large portion of Chromium's code is in shared header files that dramatically reduces the total amount of work needed. On 2017/07/05 21:50:41, brucedawson wrote: > compilation units-> translation units. Done. https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md#newcode11 docs/jumbo.md:11: Jumbo is currently implemented as a combined `gn` template and a python script. Eventually it may become at native `gn` feature. By using the template `jumbo_target`, each target will split into one action to "merge" the files and one action to compile the merged files and any files left outside the merge. On 2017/07/05 21:50:40, brucedawson wrote: > 'at native' -> 'a native' Done. https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md#newcode17 docs/jumbo.md:17: The "merge" is currently done by creating wrapper files that include the source files. On 2017/07/05 21:50:41, brucedawson wrote: > "include" or "#include" - there's a big difference. Done. #include for now. We used concatenated files for jumbo for Presto and in sumo (another build accelerator related to ninja) which has higher overhead and more complicated dependency tracking but fewer surprises. https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md#newcode23 docs/jumbo.md:23: * Everything compiles faster. When fully enabled everywhere this can **save hours** for a single full compile on a moderate computer. On 2017/07/05 21:50:41, brucedawson wrote: > -< for a single full compile on a moderate computer. > -> for a full build on a moderate computer. > > It doesn't make sense to call it a full 'compile' because it's much more than > just compiling. > > BTW, my new four-core *laptop* built Chrome in 2:04:30. Jumbo builds would have > helped, I'm sure, but I'm curious as to what computers take long enough to build > Chrome that jumbo builds could possibly save hours. Done. https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md#newcode24 docs/jumbo.md:24: * Linking is faster because there is less debug data to merge. On 2017/07/05 21:50:41, brucedawson wrote: > -< less debug data to merge. > -> less redundant data (debug information, inline functions) to merge. > > Many inline functions, especially those that are exported, are generated in > every translation unit and this also needs to be deduped by the linker. Done. https://codereview.chromium.org/2968963002/diff/1/docs/jumbo.md#newcode29 docs/jumbo.md:29: * By merging many files, symbols in different `cc` files can collide and cause compilation errors. On 2017/07/05 21:50:41, brucedawson wrote: > -< symbols in different `cc` files can collide and cause compilation errors. > -> symbols that have internal linkage in different `cc` files can collide and > cause compilation errors. Done.
On 2017/07/06 09:48:12, Daniel Bratell wrote: > Should this be linked from somewhere as well? Yes. I'm not sure where it should be linked from. Probably multiple places with one of them being the just-added reference to use_jumbo_build in the windows_build_instructions. https://cs.chromium.org/chromium/src/docs/windows_build_instructions.md?q=f:w... Adding that in some platform-neutral locations would also be good, but I don't know from where.
I would use an 80-col line length. It is true that longer lines work fine, but the Google Markdown Style Guide (https://google.github.io/styleguide/docguide/style.html) suggests using the same line length as the rest of the project, which seems like a reasonable practice. As far as links go, I would add it to the "faster builds" sections of the *_build_instructions pages for the various platforms. lgtm w/ the line length adjusted and the other comments addressed.
On 2017/07/06 22:21:01, Dirk Pranke wrote: > I would use an 80-col line length. It is true that longer lines work fine, but > the Google Markdown Style Guide > (https://google.github.io/styleguide/docguide/style.html) > suggests using the same line length as the rest of the project, which seems like > a reasonable practice. > > As far as links go, I would add it to the "faster builds" sections of the > *_build_instructions pages for the various > platforms. > > lgtm w/ the line length adjusted and the other comments addressed. How do you preview the result before committing? I mean, the only reason I have kept the long lines this long was to be able to preview the rendered result.
The CQ bit was checked by bratell@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by bratell@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> How do you preview the result before committing? I mean, the only reason I have > kept the long lines this long was to be able to preview the rendered result. I use dillinger.io to preview. See crrev.com/2959293002. It seemed to work well enough although I didn't ponder the line lengths much, so maybe we are getting more/weirder line wraps than are ideal.
On 2017/07/07 07:49:54, Daniel Bratell wrote: > How do you preview the result before committing? I mean, the only reason I have > kept the long lines this long was to be able to preview the rendered result. There's a python-based viewer in //tools/md_browser that matches the output pretty closely that you can use locally. Also, now that we're switching to Gerrit for the code reviews, there's a link near the top of the review page that you can click on to see exactly what it'll look like in Gitiles.
The CQ bit was checked by bratell@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2968963002/#ps60001 (title: "Rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499680651661640,
"parent_rev": "8e79102feb576651f4c52ef931015898478a55fe", "commit_rev":
"dbfab9c2f00000115eb304de54a7b2ed0a4663ac"}
Message was sent while issue was closed.
Description was changed from ========== Some documentation for the jumbo feature. R=dpranke@chromium.org BUG=713317 ========== to ========== Some documentation for the jumbo feature. R=dpranke@chromium.org BUG=713317 Review-Url: https://codereview.chromium.org/2968963002 Cr-Commit-Position: refs/heads/master@{#485222} Committed: https://chromium.googlesource.com/chromium/src/+/dbfab9c2f00000115eb304de54a7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/dbfab9c2f00000115eb304de54a7... |
