Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2106353004:
Implement "copy_bundle_data" tool without using a python script. (Closed)
DescriptionImplement "copy_bundle_data" tool without using a python script.
When building Chrome on iOS there are 60000+ copy_bundle_data steps,
so using a python script is slow. Instead re-implement the logic in
pure shell and using hard-links when possible.
Improve the build time of a clobber build by 4.6x (measured on a
Mac Pro using goma with a reduction from 32m24.498s to 7m1.310s
according to "time").
Commands used to time the build:
$ gn clean out/gn-Debug-iphoneos
$ time ninja -j 1000 -C out/gn-Debug-iphoneos All_iOS
BUG=621030
Committed: https://crrev.com/d5e358f61f3d6257062cdc6a6ec6a76104273ca7
Cr-Commit-Position: refs/heads/master@{#403696}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Use "pax" instead of "rsync" to use hard links when possible. #
Total comments: 4
Patch Set 3 : Quote use of $OLDPWD. #
Messages
Total messages: 26 (12 generated)
sdefresne@chromium.org changed reviewers: + dpranke@chromium.org, rsesek@chromium.org
Please take a look. This reduce the build time (but requires the dependent CL to land and be rolled in gn so not sending to the bots).
https://codereview.chromium.org/2106353004/diff/1/build/toolchain/mac/BUILD.gn File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/2106353004/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:309: _command = "if [[ -d {{source}} ]]; then " + This is missing the rm step first. https://codereview.chromium.org/2106353004/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:314: if (_convert_strings) { Should we just make string conversion a separate step at this point, and not make it a part of COPY_BUNDLE_DATA? That would align with how we do xib and xcasset compilation too.
Description was changed from ========== Implement "copy_bundle_data" tool without using a python script. When building Chrome on iOS there are 60000+ copy_bundle_data steps, so using a python script is slow. Instead re-implement the logic in pure shell and using hard-links when possible. BUG=621030 ========== to ========== Implement "copy_bundle_data" tool without using a python script. When building Chrome on iOS there are 60000+ copy_bundle_data steps, so using a python script is slow. Instead re-implement the logic in pure shell and using hard-links when possible. Improve the build time of a clobber build by Xx (measured on a Mac Pro using goma with a reduction from xx:xx:xx to yy:yy:yy). BUG=621030 ==========
https://codereview.chromium.org/2106353004/diff/1/build/toolchain/mac/BUILD.gn File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/2106353004/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:309: _command = "if [[ -d {{source}} ]]; then " + On 2016/06/30 17:28:02, Robert Sesek wrote: > This is missing the rm step first. Fixed. https://codereview.chromium.org/2106353004/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:314: if (_convert_strings) { On 2016/06/30 17:28:02, Robert Sesek wrote: > Should we just make string conversion a separate step at this point, and not > make it a part of COPY_BUNDLE_DATA? That would align with how we do xib and > xcasset compilation too. I think we should do it as a separate step, however, there are 50+ such files and I'll prefer to do it as a followup CLs as this CL is a radical improvement for build time already.
Description was changed from ========== Implement "copy_bundle_data" tool without using a python script. When building Chrome on iOS there are 60000+ copy_bundle_data steps, so using a python script is slow. Instead re-implement the logic in pure shell and using hard-links when possible. Improve the build time of a clobber build by Xx (measured on a Mac Pro using goma with a reduction from xx:xx:xx to yy:yy:yy). BUG=621030 ========== to ========== Implement "copy_bundle_data" tool without using a python script. When building Chrome on iOS there are 60000+ copy_bundle_data steps, so using a python script is slow. Instead re-implement the logic in pure shell and using hard-links when possible. Improve the build time of a clobber build by 4.6x (measured on a Mac Pro using goma with a reduction from 32m24.498s to 7m1.310s according to time). Commands used to time the build: $ gn clean out/gn-Debug-iphoneos $ time ninja -j 1000 -C out/gn-Debug-iphoneos All_iOS BUG=621030 ==========
Description was changed from ========== Implement "copy_bundle_data" tool without using a python script. When building Chrome on iOS there are 60000+ copy_bundle_data steps, so using a python script is slow. Instead re-implement the logic in pure shell and using hard-links when possible. Improve the build time of a clobber build by 4.6x (measured on a Mac Pro using goma with a reduction from 32m24.498s to 7m1.310s according to time). Commands used to time the build: $ gn clean out/gn-Debug-iphoneos $ time ninja -j 1000 -C out/gn-Debug-iphoneos All_iOS BUG=621030 ========== to ========== Implement "copy_bundle_data" tool without using a python script. When building Chrome on iOS there are 60000+ copy_bundle_data steps, so using a python script is slow. Instead re-implement the logic in pure shell and using hard-links when possible. Improve the build time of a clobber build by 4.6x (measured on a Mac Pro using goma with a reduction from 32m24.498s to 7m1.310s according to "time"). Commands used to time the build: $ gn clean out/gn-Debug-iphoneos $ time ninja -j 1000 -C out/gn-Debug-iphoneos All_iOS BUG=621030 ==========
Clever find. Wouldn't have thought of using pax in this way. https://codereview.chromium.org/2106353004/diff/1/build/toolchain/mac/BUILD.gn File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/2106353004/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:314: if (_convert_strings) { On 2016/07/01 11:12:29, sdefresne wrote: > On 2016/06/30 17:28:02, Robert Sesek wrote: > > Should we just make string conversion a separate step at this point, and not > > make it a part of COPY_BUNDLE_DATA? That would align with how we do xib and > > xcasset compilation too. > > I think we should do it as a separate step, however, there are 50+ such files > and I'll prefer to do it as a followup CLs as this CL is a radical improvement > for build time already. Yes, fine to do that in a follow up. Can you file a bug? https://codereview.chromium.org/2106353004/diff/20001/build/toolchain/mac/BUI... File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/2106353004/diff/20001/build/toolchain/mac/BUI... build/toolchain/mac/BUILD.gn:326: _copydir = "mkdir -p {{output}} && cd {{source}} && " + I assume this cd is safe because we're running in a subshell. Alternatively, could you build the absolute path using $(pwd -P) ? https://codereview.chromium.org/2106353004/diff/20001/build/toolchain/mac/BUI... build/toolchain/mac/BUILD.gn:327: "pax -rwl . \$OLDPWD/{{output}}" The substitution will handle shell escaping and quoting, but I don't think the bash variable substitution will. So this variable should probably be quoted.
I'll defer to robert here ... lgtm when he's happy.
Patchset #3 (id:40001) has been deleted
rsesek: PTAL https://codereview.chromium.org/2106353004/diff/20001/build/toolchain/mac/BUI... File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/2106353004/diff/20001/build/toolchain/mac/BUI... build/toolchain/mac/BUILD.gn:326: _copydir = "mkdir -p {{output}} && cd {{source}} && " + On 2016/07/01 15:12:17, Robert Sesek wrote: > I assume this cd is safe because we're running in a subshell. Yes, the "cd" is safe as this is only changing the path for the shell, not for ninja itself. > Alternatively, could you build the absolute path using $(pwd -P) ? I need the "cd" because if I invoke pax with a source path that is not ".", then it copies the structures in the output directory, for example, if we have the following directories: src/A file1 file2 and invoke pax like this to copy src/A to dst/A: mkdir -p dst/A && pax -rwl src/A dst/A then we'll end up with the following directory structure: dst/A/src/A file1 file2 instead of dst/A file1 file2 According to the man page, this is expected and this is why the recommend instead to use the following command: mkdir -p dst/A && cd src/A && pax -rwl . ../../dst/A So I have a choice between using $(pwd) or $OLDPWD and I chose OLDPWD because it is available and allows me to avoid invoking another subshell (so should be slightly faster as there is one less process to create). https://codereview.chromium.org/2106353004/diff/20001/build/toolchain/mac/BUI... build/toolchain/mac/BUILD.gn:327: "pax -rwl . \$OLDPWD/{{output}}" On 2016/07/01 15:12:17, Robert Sesek wrote: > The substitution will handle shell escaping and quoting, but I don't think the > bash variable substitution will. So this variable should probably be quoted. Done.
Patchset #3 (id:60001) has been deleted
LGTM. Thanks for the explanation about pax.
The CQ bit was checked by sdefresne@chromium.org
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/2106353004/#ps80001 (title: "Quote use of $OLDPWD.")
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: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
Thank you for the review during an holiday. Sending back to CQ as failure is due to infra (bot cannot start goma) that may be flaky.
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement "copy_bundle_data" tool without using a python script. When building Chrome on iOS there are 60000+ copy_bundle_data steps, so using a python script is slow. Instead re-implement the logic in pure shell and using hard-links when possible. Improve the build time of a clobber build by 4.6x (measured on a Mac Pro using goma with a reduction from 32m24.498s to 7m1.310s according to "time"). Commands used to time the build: $ gn clean out/gn-Debug-iphoneos $ time ninja -j 1000 -C out/gn-Debug-iphoneos All_iOS BUG=621030 ========== to ========== Implement "copy_bundle_data" tool without using a python script. When building Chrome on iOS there are 60000+ copy_bundle_data steps, so using a python script is slow. Instead re-implement the logic in pure shell and using hard-links when possible. Improve the build time of a clobber build by 4.6x (measured on a Mac Pro using goma with a reduction from 32m24.498s to 7m1.310s according to "time"). Commands used to time the build: $ gn clean out/gn-Debug-iphoneos $ time ninja -j 1000 -C out/gn-Debug-iphoneos All_iOS BUG=621030 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Implement "copy_bundle_data" tool without using a python script. When building Chrome on iOS there are 60000+ copy_bundle_data steps, so using a python script is slow. Instead re-implement the logic in pure shell and using hard-links when possible. Improve the build time of a clobber build by 4.6x (measured on a Mac Pro using goma with a reduction from 32m24.498s to 7m1.310s according to "time"). Commands used to time the build: $ gn clean out/gn-Debug-iphoneos $ time ninja -j 1000 -C out/gn-Debug-iphoneos All_iOS BUG=621030 ========== to ========== Implement "copy_bundle_data" tool without using a python script. When building Chrome on iOS there are 60000+ copy_bundle_data steps, so using a python script is slow. Instead re-implement the logic in pure shell and using hard-links when possible. Improve the build time of a clobber build by 4.6x (measured on a Mac Pro using goma with a reduction from 32m24.498s to 7m1.310s according to "time"). Commands used to time the build: $ gn clean out/gn-Debug-iphoneos $ time ninja -j 1000 -C out/gn-Debug-iphoneos All_iOS BUG=621030 Committed: https://crrev.com/d5e358f61f3d6257062cdc6a6ec6a76104273ca7 Cr-Commit-Position: refs/heads/master@{#403696} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d5e358f61f3d6257062cdc6a6ec6a76104273ca7 Cr-Commit-Position: refs/heads/master@{#403696} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
