Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2973603003:
Jumbo for blink/core generated files as well (saving 8 CPU minutes) (Closed)
|
Created:
3 years, 5 months ago by Daniel Bratell Modified:
3 years, 5 months ago CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionJumbo for blink/core generated files as well (saving 8 CPU minutes)
The target that compiles generated files does not use the same
template as other code in blink core so it didn't automatically become
jumbo enabled. Since it's a non-negliable part of the build time (~1%)
this patch enables jumbo for this target as well.
R=fs@opera.com
BUG=713137
Review-Url: https://codereview.chromium.org/2973603003
Cr-Commit-Position: refs/heads/master@{#485267}
Committed: https://chromium.googlesource.com/chromium/src/+/31abd9178c297b8c98cb43bf4037ff6b07ab0f83
Patch Set 1 #
Total comments: 1
Patch Set 2 : Nicer path format (and enable jumbo for dryrun testing) #
Total comments: 1
Patch Set 3 : Try to shorten jumbo gen path #
Total comments: 4
Patch Set 4 : Document why shorter paths are necessary (and good). #Patch Set 5 : Remove temporary testing code. #Messages
Total messages: 31 (20 generated)
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...
Description was changed from ========== Jumbo for blink/core generated files as well (saving 8 CPU minutes) The target that compiles generated files does not use the same template as other code in blink core so it didn't automatically become jumbo enabled. Since it's a non-negliable part of the build time (~1%) this patch enables jumbo for this target as well. R=fs@opera.com BUG=713137 ========== to ========== Jumbo for blink/core generated files as well (saving 8 CPU minutes) The target that compiles generated files does not use the same template as other code in blink core so it didn't automatically become jumbo enabled. Since it's a non-negliable part of the build time (~1%) this patch enables jumbo for this target as well. R=fs@opera.com BUG=713137 ==========
https://codereview.chromium.org/2973603003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/BUILD.gn (right): https://codereview.chromium.org/2973603003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/BUILD.gn:1088: blink_core_output_dir + "/SVGElementFactory.cpp", "$blink_core_output_dir/SVGElementFactory.cpp" appears to be a more commonly used formulation - would that work here too?
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...
https://codereview.chromium.org/2973603003/diff/20001/build/config/jumbo.gni File build/config/jumbo.gni (right): https://codereview.chromium.org/2973603003/diff/20001/build/config/jumbo.gni#... build/config/jumbo.gni:10: use_jumbo_build = !is_official_build && !(is_android && !is_clang) Must not be included in the final commit. Just for testing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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.
bratell@opera.com changed reviewers: + dpranke@chromium.org
dpranke, can you take a look at the hack in jumbo.gni? It's to work around a bug in ninja where it, in Windows, doesn't allow paths with more than 32 components. That bug will eventually be fixed when depot_tools update to a newer ninja ( https://crbug.com/738186 ).
lgtm w/ comments addressed. https://codereview.chromium.org/2973603003/diff/40001/build/config/jumbo.gni File build/config/jumbo.gni (right): https://codereview.chromium.org/2973603003/diff/40001/build/config/jumbo.gni#... build/config/jumbo.gni:10: use_jumbo_build = !is_official_build && !(is_android && !is_clang) I'm guessing this change isn't intentional (or at least, isn't meant for landing)? https://codereview.chromium.org/2973603003/diff/40001/build/config/jumbo.gni#... build/config/jumbo.gni:70: # Find the gen_target_dir directory with shortest path. This is a bit obscure. Can you add more to the comment about what this is actually doing, and why we need to do it (e.g., link to the ninja bug)? Is there maybe something we should be doing in GN to make sure we get the shortest path here, instead?
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...
https://codereview.chromium.org/2973603003/diff/40001/build/config/jumbo.gni File build/config/jumbo.gni (right): https://codereview.chromium.org/2973603003/diff/40001/build/config/jumbo.gni#... build/config/jumbo.gni:10: use_jumbo_build = !is_official_build && !(is_android && !is_clang) On 2017/07/07 20:12:10, Dirk Pranke wrote: > I'm guessing this change isn't intentional (or at least, isn't meant for > landing)? I'm adding and removing this in every CL to verify that the changes work on all platforms. It would be nice to land but I understand the need to be careful. https://codereview.chromium.org/2973603003/diff/40001/build/config/jumbo.gni#... build/config/jumbo.gni:70: # Find the gen_target_dir directory with shortest path. On 2017/07/07 20:12:10, Dirk Pranke wrote: > This is a bit obscure. Can you add more to the comment about what this is > actually doing, and why we need to do it (e.g., link to the ninja bug)? > > Is there maybe something we should be doing in GN to make sure we get the > shortest path here, instead? I've expanded the comment. The part I missed in gn for this to be smoother was a len() operator to tell me the length of a string, or actually, I'd need the number of / and \ in a string. The most generic operator that would help would be one that said if a path was a "parent" of another path.
lgtm
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/2973603003/#ps80001 (title: "Remove temporary testing code.")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bratell@opera.com
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": 80001, "attempt_start_ts": 1499693242806830,
"parent_rev": "506321bd35353b714f77e0de9f107fbc9dbf7f63", "commit_rev":
"31abd9178c297b8c98cb43bf4037ff6b07ab0f83"}
Message was sent while issue was closed.
Description was changed from ========== Jumbo for blink/core generated files as well (saving 8 CPU minutes) The target that compiles generated files does not use the same template as other code in blink core so it didn't automatically become jumbo enabled. Since it's a non-negliable part of the build time (~1%) this patch enables jumbo for this target as well. R=fs@opera.com BUG=713137 ========== to ========== Jumbo for blink/core generated files as well (saving 8 CPU minutes) The target that compiles generated files does not use the same template as other code in blink core so it didn't automatically become jumbo enabled. Since it's a non-negliable part of the build time (~1%) this patch enables jumbo for this target as well. R=fs@opera.com BUG=713137 Review-Url: https://codereview.chromium.org/2973603003 Cr-Commit-Position: refs/heads/master@{#485267} Committed: https://chromium.googlesource.com/chromium/src/+/31abd9178c297b8c98cb43bf4037... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/31abd9178c297b8c98cb43bf4037... |
