Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2985523002:
Update experimental Java 8 support to use Desugar instead of Retrolambda (Closed)
DescriptionUpdate experimental Java 8 support to use Desugar instead of Retrolambda
Our experiments show that Desugar, a Google-developed open-source tool,
outperforms Retrolambda in both APK sizes and Java method counts. This
CL replaces Retrolambda with Desugar in our experimental Java 8 support.
BUG=730711
Review-Url: https://codereview.chromium.org/2985523002
Cr-Commit-Position: refs/heads/master@{#491183}
Committed: https://chromium.googlesource.com/chromium/src/+/1a159a003ca82e0b0bd653078dbde80757bf9e2c
Patch Set 1 : Desugar :) #
Total comments: 6
Patch Set 2 : addressing comments #
Total comments: 6
Patch Set 3 : Addressing comments #Patch Set 4 : Further removing retrolambda #Patch Set 5 : Fix gn #Patch Set 6 : Rebase #
Messages
Total messages: 53 (38 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
zpeng@chromium.org changed reviewers: + agrieve@chromium.org
Hi Andrew, PTAL. Thanks!
https://codereview.chromium.org/2985523002/diff/40001/build/android/gyp/desug... File build/android/gyp/desugar.py (right): https://codereview.chromium.org/2985523002/diff/40001/build/android/gyp/desug... build/android/gyp/desugar.py:36: for path in sorted(classpath) + [android_sdk_jar]: shouldn't sort the classpath. Order could matter. https://codereview.chromium.org/2985523002/diff/40001/build/android/gyp/desug... build/android/gyp/desugar.py:60: options.bootclasspath] I don't think this is quite right. There's no reason to have both a "bootclasspath" as well as "android_sdk_jar". Use just one or the other. Note that bootclasspath implies that it's a colon:separated:list of files It doesn't make sense to use both the android .jar as well as the android .interface.jar. They are semantically the same in this context (just use the interface jar) https://codereview.chromium.org/2985523002/diff/40001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2985523002/diff/40001/build/config/android/in... build/config/android/internal_rules.gni:1184: #TODO(zpeng): Remove Retrolambda when Desugar is enabled. Can we do this now rather than checking in an if (false) statement?
Patchset #2 (id:60001) has been deleted
Thanks Andrew! PTAL https://codereview.chromium.org/2985523002/diff/40001/build/android/gyp/desug... File build/android/gyp/desugar.py (right): https://codereview.chromium.org/2985523002/diff/40001/build/android/gyp/desug... build/android/gyp/desugar.py:36: for path in sorted(classpath) + [android_sdk_jar]: On 2017/07/20 00:26:51, agrieve wrote: > shouldn't sort the classpath. Order could matter. Done. Thanks! I misunderstood "ordered" in the parameter description as "alphabetically ordered". My bad https://codereview.chromium.org/2985523002/diff/40001/build/android/gyp/desug... build/android/gyp/desugar.py:60: options.bootclasspath] On 2017/07/20 00:26:51, agrieve wrote: > I don't think this is quite right. There's no reason to have both a > "bootclasspath" as well as "android_sdk_jar". Use just one or the other. > > Note that bootclasspath implies that it's a colon:separated:list of files > > It doesn't make sense to use both the android .jar as well as the android > .interface.jar. They are semantically the same in this context (just use the > interface jar) Done. https://codereview.chromium.org/2985523002/diff/40001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2985523002/diff/40001/build/config/android/in... build/config/android/internal_rules.gni:1184: #TODO(zpeng): Remove Retrolambda when Desugar is enabled. On 2017/07/20 00:26:52, agrieve wrote: > Can we do this now rather than checking in an if (false) statement? Done.
https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/desug... File build/android/gyp/desugar.py (right): https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/desug... build/android/gyp/desugar.py:18: _DESUGAR_JAR_PATH = os.path.normpath(os.path.join( list this either in depfile_deps or in GN's input array for the action. https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/desug... build/android/gyp/desugar.py:69: depfile_deps=input_paths) nit: don't pass input_jar and bootclasspath_entry as depfile_deps. The idea is to only list inputs that GN hasn't already listed. https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/retro... File build/android/gyp/retrolambda.py (right): https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/retro... build/android/gyp/retrolambda.py:66: depfile_deps=input_paths) You should delete retrolambda in this change as well (rather than deleting it in a follow-up). That way only a single commit needs to be reverted if it doesn't work.
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Thanks Andrew! PTAL https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/desug... File build/android/gyp/desugar.py (right): https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/desug... build/android/gyp/desugar.py:18: _DESUGAR_JAR_PATH = os.path.normpath(os.path.join( On 2017/07/20 18:24:09, agrieve wrote: > list this either in depfile_deps or in GN's input array for the action. Done. https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/desug... build/android/gyp/desugar.py:69: depfile_deps=input_paths) On 2017/07/20 18:24:09, agrieve wrote: > nit: don't pass input_jar and bootclasspath_entry as depfile_deps. The idea is > to only list inputs that GN hasn't already listed. Done. https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/retro... File build/android/gyp/retrolambda.py (right): https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/retro... build/android/gyp/retrolambda.py:66: depfile_deps=input_paths) On 2017/07/20 18:24:09, agrieve wrote: > You should delete retrolambda in this change as well (rather than deleting it in > a follow-up). That way only a single commit needs to be reverted if it doesn't > work. Done.
On 2017/07/20 19:40:55, F wrote: > Thanks Andrew! PTAL > > https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/desug... > File build/android/gyp/desugar.py (right): > > https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/desug... > build/android/gyp/desugar.py:18: _DESUGAR_JAR_PATH = > os.path.normpath(os.path.join( > On 2017/07/20 18:24:09, agrieve wrote: > > list this either in depfile_deps or in GN's input array for the action. > > Done. > > https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/desug... > build/android/gyp/desugar.py:69: depfile_deps=input_paths) > On 2017/07/20 18:24:09, agrieve wrote: > > nit: don't pass input_jar and bootclasspath_entry as depfile_deps. The idea is > > to only list inputs that GN hasn't already listed. > > Done. > > https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/retro... > File build/android/gyp/retrolambda.py (right): > > https://codereview.chromium.org/2985523002/diff/80001/build/android/gyp/retro... > build/android/gyp/retrolambda.py:66: depfile_deps=input_paths) > On 2017/07/20 18:24:09, agrieve wrote: > > You should delete retrolambda in this change as well (rather than deleting it > in > > a follow-up). That way only a single commit needs to be reverted if it doesn't > > work. > > Done. lgtm!
Hi Andrew, PTAL. Thanks! Now this CL completely removes retrolambda
The CQ bit was checked by zpeng@chromium.org 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
On 2017/07/28 19:19:16, F wrote: > Hi Andrew, PTAL. Thanks! > > Now this CL completely removes retrolambda lgtm once gn gen is fixed up :)
The CQ bit was checked by zpeng@chromium.org 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...
Patchset #5 (id:170001) has been deleted
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:190001) has been deleted
The CQ bit was checked by zpeng@chromium.org 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...
Or move the variable outside https://codereview.chromium.org/2985523002/diff/210001/build/config/android/i... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2985523002/diff/210001/build/config/android/i... build/config/android/internal_rules.gni:1263: if (defined(invoker.alternative_android_sdk_ijar)) { assert(!defined(invoker.alternative_android_sdk_ijar) || invoker.alternative_android_sdk_ijar != "") assert(!defined(invoker.alternative_android_sdk_ijar_dep) || invoker.alternative_android_sdk_ijar_dep != "")
Patchset #5 (id:210001) has been deleted
The CQ bit was checked by zpeng@chromium.org 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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by zpeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2985523002/#ps250001 (title: "Rebase")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by zpeng@chromium.org 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...
zpeng@chromium.org changed reviewers: + thakis@chromium.org
+thakis@ for third_party Hi Nico, PTAL. Thanks!
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by zpeng@chromium.org 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 zpeng@chromium.org
The CQ bit was checked by zpeng@chromium.org
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": 250001, "attempt_start_ts": 1501619673801620,
"parent_rev": "6c760df2f6a9f5d1ce7051be6acfeebc26919774", "commit_rev":
"fc8a4fc65fa7235a4a3b3d8c62b8eab7876f2181"}
CQ is committing da patch.
Bot data: {"patchset_id": 250001, "attempt_start_ts": 1501619673801620,
"parent_rev": "34cf3f816d0d4c4986116a63405cdc34f30a07dc", "commit_rev":
"e3f5fbaf082230b29372b763e222d5fdc0e328b0"}
CQ is committing da patch.
Bot data: {"patchset_id": 250001, "attempt_start_ts": 1501619673801620,
"parent_rev": "baca0c48578ca9509e5aa4bf94a1083f80106ca3", "commit_rev":
"1a159a003ca82e0b0bd653078dbde80757bf9e2c"}
Message was sent while issue was closed.
Description was changed from ========== Update experimental Java 8 support to use Desugar instead of Retrolambda Our experiments show that Desugar, a Google-developed open-source tool, outperforms Retrolambda in both APK sizes and Java method counts. This CL replaces Retrolambda with Desugar in our experimental Java 8 support. BUG=730711 ========== to ========== Update experimental Java 8 support to use Desugar instead of Retrolambda Our experiments show that Desugar, a Google-developed open-source tool, outperforms Retrolambda in both APK sizes and Java method counts. This CL replaces Retrolambda with Desugar in our experimental Java 8 support. BUG=730711 Review-Url: https://codereview.chromium.org/2985523002 Cr-Commit-Position: refs/heads/master@{#491183} Committed: https://chromium.googlesource.com/chromium/src/+/1a159a003ca82e0b0bd653078dbd... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:250001) as https://chromium.googlesource.com/chromium/src/+/1a159a003ca82e0b0bd653078dbd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
