Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2048163002:
Change //build/config/compiler:optimize_max to use -O3. (Closed)
|
Created:
4 years, 6 months ago by Dirk Pranke Modified:
4 years, 6 months ago CC:
chromium-reviews, jochen (gone - plz use gerrit), Michael Hablich, kcc1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange //build/config/compiler:optimize_max to use -O3.
Certain components (e.g., v8) really want to be compiled with -O3,
but the current ":optimize_max" setting just used -O2. Since "max"
should theoretically mean "max", let's try making it be -O3 across
the board and see what happens.
R=brettw@chromium.org
BUG=616031
Committed: https://crrev.com/96a6dfa2c30ab9b22abd20c87ed0e0d6ae41c40e
Cr-Commit-Position: refs/heads/master@{#398704}
Patch Set 1 #
Messages
Total messages: 19 (7 generated)
Description was changed from ========== Change //build/config/compiler:optimize_max to use -O3. Certain components (e.g., v8) really want to be compiled with -O3, but the current ":optimize_max" setting just used -O2. Since "max" should theoretically mean "max", let's try making it be -O3 across the board and see what happens. R=brettw@chromium.org BUG=616031 ========== to ========== Change //build/config/compiler:optimize_max to use -O3. Certain components (e.g., v8) really want to be compiled with -O3, but the current ":optimize_max" setting just used -O2. Since "max" should theoretically mean "max", let's try making it be -O3 across the board and see what happens. R=brettw@chromium.org BUG=616031 ==========
The CQ bit was checked by dpranke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2048163002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2048163002/1
Message was sent while issue was closed.
Description was changed from ========== Change //build/config/compiler:optimize_max to use -O3. Certain components (e.g., v8) really want to be compiled with -O3, but the current ":optimize_max" setting just used -O2. Since "max" should theoretically mean "max", let's try making it be -O3 across the board and see what happens. R=brettw@chromium.org BUG=616031 ========== to ========== Change //build/config/compiler:optimize_max to use -O3. Certain components (e.g., v8) really want to be compiled with -O3, but the current ":optimize_max" setting just used -O2. Since "max" should theoretically mean "max", let's try making it be -O3 across the board and see what happens. R=brettw@chromium.org BUG=616031 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Change //build/config/compiler:optimize_max to use -O3. Certain components (e.g., v8) really want to be compiled with -O3, but the current ":optimize_max" setting just used -O2. Since "max" should theoretically mean "max", let's try making it be -O3 across the board and see what happens. R=brettw@chromium.org BUG=616031 ========== to ========== Change //build/config/compiler:optimize_max to use -O3. Certain components (e.g., v8) really want to be compiled with -O3, but the current ":optimize_max" setting just used -O2. Since "max" should theoretically mean "max", let's try making it be -O3 across the board and see what happens. R=brettw@chromium.org BUG=616031 Committed: https://crrev.com/96a6dfa2c30ab9b22abd20c87ed0e0d6ae41c40e Cr-Commit-Position: refs/heads/master@{#398704} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/96a6dfa2c30ab9b22abd20c87ed0e0d6ae41c40e Cr-Commit-Position: refs/heads/master@{#398704}
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Message was sent while issue was closed.
Thanks for the switch. cc sanitizer folks. Is this used for sanitizer builds in release mode as well? In our old gyp config was a comment that O3 and sanitizers is not supported. Maybe that doesn't hold anymore, can anybody confirm?
Message was sent while issue was closed.
On 2016/06/09 06:52:35, Michael Achenbach wrote: > Thanks for the switch. > > cc sanitizer folks. Is this used for sanitizer builds in release mode as well? > In our old gyp config was a comment that O3 and sanitizers is not supported. > Maybe that doesn't hold anymore, can anybody confirm? Well, -O3 is certainly compatible with sanitizers, but it can mask certain reports or make them less comprehensible. Is it really critical for v8 etc. to be built with -O3 when e.g. ASan or TSan are used?
Message was sent while issue was closed.
On 2016/06/09 11:37:48, Alexander Potapenko wrote: > On 2016/06/09 06:52:35, Michael Achenbach wrote: > > Thanks for the switch. > > > > cc sanitizer folks. Is this used for sanitizer builds in release mode as well? > > In our old gyp config was a comment that O3 and sanitizers is not supported. > > Maybe that doesn't hold anymore, can anybody confirm? > > Well, -O3 is certainly compatible with sanitizers, but it can mask certain > reports or make them less comprehensible. > Is it really critical for v8 etc. to be built with -O3 when e.g. ASan or TSan > are used? No, not at all. We just want to get O3 in normal release builds. But I'm not even sure the optimize_max config is used for sanitizers. Maybe there is a condition elsewhere that only uses optimize_on? Otherwise we could add a condition here.
Message was sent while issue was closed.
On 2016/06/09 11:47:50, Michael Achenbach wrote: > On 2016/06/09 11:37:48, Alexander Potapenko wrote: > > On 2016/06/09 06:52:35, Michael Achenbach wrote: > > > Thanks for the switch. > > > > > > cc sanitizer folks. Is this used for sanitizer builds in release mode as > well? > > > In our old gyp config was a comment that O3 and sanitizers is not supported. > > > Maybe that doesn't hold anymore, can anybody confirm? > > > > Well, -O3 is certainly compatible with sanitizers, but it can mask certain > > reports or make them less comprehensible. > > Is it really critical for v8 etc. to be built with -O3 when e.g. ASan or TSan > > are used? > > No, not at all. We just want to get O3 in normal release builds. But I'm not > even sure the optimize_max config is used for sanitizers. Maybe there is a > condition elsewhere that only uses optimize_on? Otherwise we could add a > condition here. The sanitizers work with -O3, but we don't test this configuration hard enough. And just this week we were fighting with a bug in msan that happens only at O3. So, unless you have strong reasons, please don't change the sanitizers' opt level.
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2062433002/ by dpranke@chromium.org. The reason for reverting is: Reverting, this caused a size regression for cronet that is more important to them than any likely perf gains. I'll re-land a different CL that's less broad.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
