Turn maximize speed on for skia in official build

Turn maximize speed on for skia in official build

BUG=543583
R=hendrikw

Committed: https://crrev.com/1b08081a7a1f438999aa401417538d73afbcdf89
Cr-Commit-Position: refs/heads/master@{#357096}

[mtklein, 2015-10-27 15:43:18]

On 2015/10/27 15:20:37, Lof wrote:

Seems like this would want to be a GYP/GN change?  Switch us over to /O2 at the
source, rather than override it with a pragma?

[Lof, 2015-10-27 16:12:31]

On 2015/10/27 15:43:18, mtklein wrote:
> Seems like this would want to be a GYP/GN change?  Switch us over to /O2 at
the
> source, rather than override it with a pragma?

I think so. But I try to fix only for msvs 2013.
I'm afraid to change GYP/GN files because there are a lot of conditions...

[Lof, 2015-10-27 16:26:50]

lof84@yandex-team.ru changed reviewers:
+ mtklein@chromium.org

[hendrikw, 2015-10-27 16:41:47]

On 2015/10/27 16:12:31, Lof wrote:
> On 2015/10/27 15:43:18, mtklein wrote:
> > Seems like this would want to be a GYP/GN change?  Switch us over to /O2 at
> the
> > source, rather than override it with a pragma?
> 
> I think so. But I try to fix only for msvs 2013.
> I'm afraid to change GYP/GN files because there are a lot of conditions...

Yeah, GYP files are not fun, but I agree, it should be fixed via GYP (and GN
needs updating as well).

You can use the condition MSVS_VERSION == "2013" in the gyp file to limit it.

If you have trouble getting this working, I can try to help, or try to do it.

[Lof, 2015-10-27 17:10:25]

On 2015/10/27 16:41:47, hendrikw wrote:
> If you have trouble getting this working, I can try to help, or try to do it.
I need help :)
I checked it by 'ninja -d keeprsp -C ./ cc_unittests' - skia is compiled with
/O1

[Lof, 2015-10-27 18:12:12]

It works! But how can I specify release?

[hendrikw, 2015-10-27 18:19:36]

On 2015/10/27 18:12:12, Lof wrote:
> It works! But how can I specify release?

Cool!

If you look at build\common.gypi : 5575, it states:

              # In official builds, targets can self-select an optimization
              # level by defining a variable named 'optimize', and setting it
              # to one of
              # - "size", optimizes for minimal code size - the default.
              # - "speed", optimizes for speed over code size.
              # - "max", whole program optimization and link-time code
              #   generation. This is very expensive and should be used
              #   sparingly.

So maybe the correct way to handle this is to use this?

As an example, if you look in base\base.gyp, you'll see that we add 'optimize':
'max' to the variables.

I don't know yet for sure, but this probably handles debug/release as well.

[hendrikw, 2015-10-27 18:21:19]

On 2015/10/27 18:19:36, hendrikw wrote:
> On 2015/10/27 18:12:12, Lof wrote:
> > It works! But how can I specify release?
> 
> Cool!
> 
> If you look at build\common.gypi : 5575, it states:
> 
>               # In official builds, targets can self-select an optimization
>               # level by defining a variable named 'optimize', and setting it
>               # to one of
>               # - "size", optimizes for minimal code size - the default.
>               # - "speed", optimizes for speed over code size.
>               # - "max", whole program optimization and link-time code
>               #   generation. This is very expensive and should be used
>               #   sparingly.
> 
> So maybe the correct way to handle this is to use this?
> 
> As an example, if you look in base\base.gyp, you'll see that we add
'optimize':
> 'max' to the variables.
> 
> I don't know yet for sure, but this probably handles debug/release as well.

Err, 'optimize': 'max' is just an example, I haven't tried to figure out which
optimize setting you'd want.

[Lof, 2015-10-27 19:16:52]

optimize:max works for cc/, but doesn't work for skia :(

[mtklein, 2015-10-27 20:08:56]

The CQ bit was checked by mtklein@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-27 20:10:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1410883008/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1410883008/60001

[commit-bot: I haz the power, 2015-10-27 20:10:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-27 20:10:32]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Lof, 2015-10-27 21:38:14]

It looks like right fix. But it is common.gypi...

[hendrikw, 2015-10-27 21:43:01]

On 2015/10/27 21:38:14, Lof wrote:
> It looks like right fix. But it is common.gypi...

hm, yeah, that scares me.  you're changing the default of any target not
overriding that settings, which is most of them :(

It's likely that your first attempt changed the var in the wrong scope. Changing
settings in GYP can be a huge pain.

I'll see if I can figure out how to limit the setting to skia's target.

[Lof, 2015-10-27 21:52:22]

On 2015/10/27 21:43:01, hendrikw wrote:
> I'll see if I can figure out how to limit the setting to skia's target.
Yes, we can fix the bug in skia.
But we loose performance in official build due to non optimal settings.
A lot of files are compiled with /O1 instead of /O2.
For example, all files in gpu/ directory.

[hendrikw, 2015-10-27 21:59:52]

On 2015/10/27 21:52:22, Lof wrote:
> On 2015/10/27 21:43:01, hendrikw wrote:
> > I'll see if I can figure out how to limit the setting to skia's target.
> Yes, we can fix the bug in skia.
> But we loose performance in official build due to non optimal settings.
> A lot of files are compiled with /O1 instead of /O2.
> For example, all files in gpu/ directory.

We need to limit the scope of this change to skia. Making a global change like
this carries a lot of risks :)

It's also probable that we made the conscious decision to favor size over speed
by default.

[Lof, 2015-10-27 22:33:30]

On 2015/10/27 21:59:52, hendrikw wrote:
> We need to limit the scope of this change to skia. Making a global change like
> this carries a lot of risks :)
> 
> It's also probable that we made the conscious decision to favor size over
speed
> by default.

I agree about risks :)
But Release builds with /O2, and Release is tested.
In Official build we have mix /O2 and /O1.
May be we have the same problem with codegeneration in others files.
PS. It was a long time ago...
https://code.google.com/p/chromium/issues/detail?id=108167
And skia: https://codereview.chromium.org/8965045/
I can't understand why they choose size instead of speed.

[hendrikw, 2015-10-27 22:45:40]

On 2015/10/27 22:33:30, Lof wrote:
> On 2015/10/27 21:59:52, hendrikw wrote:
> > We need to limit the scope of this change to skia. Making a global change
like
> > this carries a lot of risks :)
> > 
> > It's also probable that we made the conscious decision to favor size over
> speed
> > by default.
> 
> I agree about risks :)
> But Release builds with /O2, and Release is tested.
> In Official build we have mix /O2 and /O1.
> May be we have the same problem with codegeneration in others files.
> PS. It was a long time ago...
> https://code.google.com/p/chromium/issues/detail?id=108167
> And skia: https://codereview.chromium.org/8965045/
> I can't understand why they choose size instead of speed.

Interesting, I guess we should figure out if this should change.  Still for this
cl, I'd like to limit the scope.  I'll start a discussion in chrome-dev.

[Lof, 2015-10-27 22:47:50]

On 2015/10/27 22:45:40, hendrikw wrote:
> Interesting, I guess we should figure out if this should change.  Still for
this
> cl, I'd like to limit the scope.  I'll start a discussion in chrome-dev.
Ok.

[Lof, 2015-10-29 15:35:44]

Hi!
Fix for skia only.

[hendrikw, 2015-10-29 16:24:37]

On 2015/10/29 15:35:44, Lof wrote:
> Hi!
> Fix for skia only.

Thanks, however, I built this locally (Official + 32bit), and when selecting
"multiply" on http://codepen.io/adobe/pen/FeiCp, I'm getting a solid color.

I checked the generated .ninja files, and /O2 is being applied to skia's files.

I'm not sure why it doesn't work.  Is the output from the codepen example
working on your system?

[PhistucK, 2015-10-29 16:30:22]

phistuck@gmail.com changed reviewers:
+ phistuck@gmail.com

[Lof, 2015-10-29 16:34:57]

On 2015/10/29 16:24:37, hendrikw wrote:
> Thanks, however, I built this locally (Official + 32bit), and when selecting
> "multiply" on http://codepen.io/adobe/pen/FeiCp, I'm getting a solid color.
> 
> I checked the generated .ninja files, and /O2 is being applied to skia's
files.
> 
> I'm not sure why it doesn't work.  Is the output from the codepen example
> working on your system?
Hm... It works on my system.
I try to do clean build now (2h for linking blink...)

[hendrikw, 2015-10-29 17:10:24]

On 2015/10/29 16:34:57, Lof wrote:
> On 2015/10/29 16:24:37, hendrikw wrote:
> > Thanks, however, I built this locally (Official + 32bit), and when selecting
> > "multiply" on http://codepen.io/adobe/pen/FeiCp, I'm getting a solid color.
> > 
> > I checked the generated .ninja files, and /O2 is being applied to skia's
> files.
> > 
> > I'm not sure why it doesn't work.  Is the output from the codepen example
> > working on your system?
> Hm... It works on my system.
> I try to do clean build now (2h for linking blink...)

Ok, in parallel, I'll also check the prior change (build everything with 'max').

[Lof, 2015-10-29 19:19:47]

I checked clean build.
Only skia_opts.ninja has /O1 :(
All others skia*.ninja files have /O2

[hendrikw, 2015-10-29 19:51:40]

On 2015/10/29 19:19:47, Lof wrote:
> I checked clean build.
> Only skia_opts.ninja has /O1 :(
> All others skia*.ninja files have /O2

Oh?  That's interesting.  So we need to find out what makes that .ninja file.

I tried applying max as the default, and it does work in that case.

[hendrikw, 2015-10-29 19:54:52]

On 2015/10/29 19:51:40, hendrikw wrote:
> On 2015/10/29 19:19:47, Lof wrote:
> > I checked clean build.
> > Only skia_opts.ninja has /O1 :(
> > All others skia*.ninja files have /O2
> 
> Oh?  That's interesting.  So we need to find out what makes that .ninja file.
> 
> I tried applying max as the default, and it does work in that case.

Ok, you'll need to add the 'max' to skia_library_opts.gyp instead.

[Lof, 2015-10-29 20:31:14]

It works! Thank you.
But I try to do clean build again with final patch set.

[hendrikw, 2015-10-29 21:45:24]

On 2015/10/29 20:31:14, Lof wrote:
> It works! Thank you.
> But I try to do clean build again with final patch set.

Awesome!  I'll try it out here as well.

[hendrikw, 2015-10-29 22:36:30]

On 2015/10/29 21:45:24, hendrikw wrote:
> On 2015/10/29 20:31:14, Lof wrote:
> > It works! Thank you.
> > But I try to do clean build again with final patch set.
> 
> Awesome!  I'll try it out here as well.

Works!

The only thing I'm not sure of is, do we want to limit this to the ops gyp?

lgtm, but I'm not an owner.

mtklein, PTAL, thanks!

[mtklein_C, 2015-10-30 02:30:29]

lgtm

It'd be nice to leave a note in each of these files like,

# The optimize: 'max' scattered throughout are particularly important when
compiled by MSVC 2013,
# which seems to mis-link-time-compile code that's built with different
optimization levels.

...or whatever's the correct explanation (is my understanding right here?).

[Lof, 2015-10-30 11:42:34]

I checked clean build with all use-cases from http://crbug.com/543583
All are fixed.
I added comments.

[Lof, 2015-10-30 11:42:41]

The CQ bit was checked by lof84@yandex-team.ru

[Lof, 2015-10-30 11:42:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
hendrikw@chromium.org, mtklein@chromium.org
Link to the patchset: https://codereview.chromium.org/1410883008/#ps130001
(title: "comments")

[commit-bot: I haz the power, 2015-10-30 11:43:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1410883008/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1410883008/130001

[commit-bot: I haz the power, 2015-10-30 13:29:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-30 13:29:27]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Lof, 2015-10-30 13:31:21]

The CQ bit was checked by lof84@yandex-team.ru

[commit-bot: I haz the power, 2015-10-30 13:31:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1410883008/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1410883008/130001

[commit-bot: I haz the power, 2015-10-30 14:21:33]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2015-10-30 14:22:12]

Patchset 8 (id:??) landed as
https://crrev.com/1b08081a7a1f438999aa401417538d73afbcdf89
Cr-Commit-Position: refs/heads/master@{#357096}

[Nico, 2016-03-31 22:33:00]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-03-31 22:33:01]

This added about 600kB to the Windows binary size. From the bug, it sounds like
this was for a bugfix. Is there a way we can do this without adding over half a
meg to chrome.dll?

[Nico, 2016-03-31 22:33:13]

see
https://chromeperf.appspot.com/report?sid=2f615b8d275e364e6dd6b53b1abb78c4698...

[hendrikw, 2016-03-31 22:43:10]

On 2016/03/31 22:33:13, Nico wrote:
> see
>
https://chromeperf.appspot.com/report?sid=2f615b8d275e364e6dd6b53b1abb78c4698...

If I recall correctly, there was a compiler bug in msvc2013.  You could try
turning the optimization off in 2015, though you might end up sacrificing
performance for 1/2 a meg on a 33 meg dll, or maybe not, but when you try,
you'll need to watch the performance graphs.