Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
199633004:
Put the syzygy targets behind a 'syzygy_optimize' flag. (Closed)
|
Created:
6 years, 9 months ago by Sébastien Marchand Modified:
6 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionPut the syzygy targets behind a 'syzygy_optimize' flag.
R=chrisha@chromium.org, pkasting@chromium.org
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257972
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Address Chris' nits. #
Total comments: 2
Patch Set 3 : Address Chris' comment. #
Total comments: 8
Patch Set 4 : Address Scott's comments. #Patch Set 5 : Rebase. #
Messages
Total messages: 25 (0 generated)
PTAL.
https://codereview.chromium.org/199633004/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/199633004/diff/20001/build/common.gypi#newcod... build/common.gypi:838: }], Is there any meaningful way for us to explode with a message if both syzygy_optimize and syzyasan are set? https://codereview.chromium.org/199633004/diff/20001/chrome/chrome_syzygy.gyp File chrome/chrome_syzygy.gyp (right): https://codereview.chromium.org/199633004/diff/20001/chrome/chrome_syzygy.gyp... chrome/chrome_syzygy.gyp:52: '(syzygy_optimize==1 or syzyasan==1)', { A comment to describe this logic? It's a bit tortuous.... :) https://codereview.chromium.org/199633004/diff/20001/chrome/chrome_syzygy.gypi File chrome/chrome_syzygy.gypi (right): https://codereview.chromium.org/199633004/diff/20001/chrome/chrome_syzygy.gyp... chrome/chrome_syzygy.gypi:46: ['syzyasan==1', { Should syzygy_optimize and syzyasan be mutually exclusive? Do you want to check syzyasan==1 and syzygy_optimize==0 like above?
Thanks, PTAnL. https://codereview.chromium.org/199633004/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/199633004/diff/20001/build/common.gypi#newcod... build/common.gypi:838: }], On 2014/03/14 22:01:36, chrisha wrote: > Is there any meaningful way for us to explode with a message if both > syzygy_optimize and syzyasan are set? No (afaik), I've added an extra check here but preventing both variables to be set seems tricky...(i.e. we'll have to run a command that will fail and print the message) https://codereview.chromium.org/199633004/diff/20001/chrome/chrome_syzygy.gyp File chrome/chrome_syzygy.gyp (right): https://codereview.chromium.org/199633004/diff/20001/chrome/chrome_syzygy.gyp... chrome/chrome_syzygy.gyp:52: '(syzygy_optimize==1 or syzyasan==1)', { On 2014/03/14 22:01:36, chrisha wrote: > A comment to describe this logic? It's a bit tortuous.... :) I've refactored this file a little bit. https://codereview.chromium.org/199633004/diff/20001/chrome/chrome_syzygy.gypi File chrome/chrome_syzygy.gypi (right): https://codereview.chromium.org/199633004/diff/20001/chrome/chrome_syzygy.gyp... chrome/chrome_syzygy.gypi:46: ['syzyasan==1', { On 2014/03/14 22:01:36, chrisha wrote: > Should syzygy_optimize and syzyasan be mutually exclusive? Do you want to check > syzyasan==1 and syzygy_optimize==0 like above? Done.
lgtm with a nit... maybe get a second opinion from somebody in the know about the phony target type trick... https://codereview.chromium.org/199633004/diff/60001/chrome/chrome_syzygy.gyp File chrome/chrome_syzygy.gyp (right): https://codereview.chromium.org/199633004/diff/60001/chrome/chrome_syzygy.gyp... chrome/chrome_syzygy.gyp:10: 'type': 'must_not_use_syzygy_optimize_and_syzyasan_together', A comment as to what this is for, and why it works.
Thanks, I'll try to find an appropriate second reviewer. https://codereview.chromium.org/199633004/diff/60001/chrome/chrome_syzygy.gyp File chrome/chrome_syzygy.gyp (right): https://codereview.chromium.org/199633004/diff/60001/chrome/chrome_syzygy.gyp... chrome/chrome_syzygy.gyp:10: 'type': 'must_not_use_syzygy_optimize_and_syzyasan_together', On 2014/03/17 18:52:13, chrisha wrote: > A comment as to what this is for, and why it works. Done.
Hey Scott, can you quickly review the gyp changes in this CL, or point me to someone that could help me to make sure that I'm not doing anything too silly here ? Thanks !
https://codereview.chromium.org/199633004/diff/80001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/199633004/diff/80001/build/common.gypi#newcod... build/common.gypi:1297: 'syzygy_optimize%': 1, I'm not sure if this setting gets copied out? But I guess it's working? https://codereview.chromium.org/199633004/diff/80001/chrome/chrome_syzygy.gyp File chrome/chrome_syzygy.gyp (right): https://codereview.chromium.org/199633004/diff/80001/chrome/chrome_syzygy.gyp... chrome/chrome_syzygy.gyp:6: ['syzyasan==1 and syzygy_optimize==1', { I don't think this block is necessary, we have many settings combinations that just won't work together, but we don't try to guard them. https://codereview.chromium.org/199633004/diff/80001/chrome/chrome_syzygy.gyp... chrome/chrome_syzygy.gyp:66: '(syzygy_optimize==1 or syzyasan==1)', { Wow, I stared at that for a while. I think it's ok? https://codereview.chromium.org/199633004/diff/80001/chrome/chrome_syzygy.gypi File chrome/chrome_syzygy.gypi (right): https://codereview.chromium.org/199633004/diff/80001/chrome/chrome_syzygy.gyp... chrome/chrome_syzygy.gypi:20: ['syzyasan!=1 and syzygy_optimize==1', { nit; can you use either !=1 or ==0 consistently for this and the one below? (don't care which)
Thanks Scott, PTAnL ? https://codereview.chromium.org/199633004/diff/80001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/199633004/diff/80001/build/common.gypi#newcod... build/common.gypi:1297: 'syzygy_optimize%': 1, On 2014/03/18 17:21:13, scottmg wrote: > I'm not sure if this setting gets copied out? But I guess it's working? Yeah it works, I guess that it's because it's the last place where I set it in this file... Not super elegant but I don't know if there's a better solution. https://codereview.chromium.org/199633004/diff/80001/chrome/chrome_syzygy.gyp File chrome/chrome_syzygy.gyp (right): https://codereview.chromium.org/199633004/diff/80001/chrome/chrome_syzygy.gyp... chrome/chrome_syzygy.gyp:6: ['syzyasan==1 and syzygy_optimize==1', { On 2014/03/18 17:21:13, scottmg wrote: > I don't think this block is necessary, we have many settings combinations that > just won't work together, but we don't try to guard them. Removed, but it'll be nice to have some assertions features in Chrome to be able to do this properly :) https://codereview.chromium.org/199633004/diff/80001/chrome/chrome_syzygy.gyp... chrome/chrome_syzygy.gyp:66: '(syzygy_optimize==1 or syzyasan==1)', { On 2014/03/18 17:21:13, scottmg wrote: > Wow, I stared at that for a while. I think it's ok? Yeah... It's pretty complex, I've added the truth table and I've been able to simplify this expression a little bit (but this can break if we set both syzyasan and syzygy_optimize). https://codereview.chromium.org/199633004/diff/80001/chrome/chrome_syzygy.gypi File chrome/chrome_syzygy.gypi (right): https://codereview.chromium.org/199633004/diff/80001/chrome/chrome_syzygy.gyp... chrome/chrome_syzygy.gypi:20: ['syzyasan!=1 and syzygy_optimize==1', { On 2014/03/18 17:21:13, scottmg wrote: > nit; can you use either !=1 or ==0 consistently for this and the one below? > (don't care which) Done.
lgtm
Thanks Scott ! Committing.
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/199633004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for build/common.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file build/common.gypi
Hunk #1 succeeded at 521 (offset 3 lines).
Hunk #2 FAILED at 821.
Hunk #3 succeeded at 953 (offset 11 lines).
Hunk #4 succeeded at 1302 with fuzz 1 (offset 10 lines).
1 out of 4 hunks FAILED -- saving rejects to file build/common.gypi.rej
Patch: build/common.gypi
Index: build/common.gypi
diff --git a/build/common.gypi b/build/common.gypi
index
c79e8e76ee7ce79ea4b8f3becf125a1544c84c49..32f85193e9ce631eccff1e0b39998ebf2416e47b
100644
--- a/build/common.gypi
+++ b/build/common.gypi
@@ -518,6 +518,9 @@
'enable_enhanced_bookmarks%': 0,
'enable_hangout_services_extension%': 0,
+ # Enable the Syzygy optimization step.
+ 'syzygy_optimize%': 0,
+
'conditions': [
# A flag for POSIX platforms
['OS=="win"', {
@@ -818,9 +821,9 @@
'enable_printing%': 0,
}],
- # By default, use ICU data file (icudtl.dat) on all platforms
- # except when building Android WebView.
- # TODO(jshin): Handle 'use_system_icu' on Linux (Chromium).
+ # By default, use ICU data file (icudtl.dat) on all platforms
+ # except when building Android WebView.
+ # TODO(jshin): Handle 'use_system_icu' on Linux (Chromium).
['android_webview_build==0', {
'icu_use_data_file_flag%' : 1,
}, {
@@ -939,6 +942,7 @@
'asan%': '<(asan)',
'asan_coverage%': '<(asan_coverage)',
'syzyasan%': '<(syzyasan)',
+ 'syzygy_optimize%': '<(syzygy_optimize)',
'lsan%': '<(lsan)',
'msan%': '<(msan)',
'msan_blacklist%': '<(msan_blacklist)',
@@ -1288,6 +1292,13 @@
'enable_new_gamepad_api%': 1,
'conditions': [
+ # Enable the Syzygy optimization step for the official builds.
+ ['OS=="win" and buildtype=="Official" and syzyasan!=1', {
+ 'syzygy_optimize%': 1,
+ }, {
+ 'syzygy_optimize%': 0,
+ }],
+
# The version of GCC in use, set later in platforms that use GCC and have
# not explicitly chosen to build with clang. Currently, this means all
# platforms except Windows, Mac and iOS.
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/199633004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/199633004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/199633004/120001
Message was sent while issue was closed.
Change committed as 257972 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
