base/allocator: Hardcode TCMalloc optimization level to 2 on Windows.

base/allocator: Hardcode TCMalloc optimization level to 2 on Windows.

TCMalloc is very slow in debug mode. Testing this out on Linux demonstrates
clear wins, so I'm hopeful Windows will similarly improve. This change mostly
copies the same configuration from ffmpeg, which unifies the setting of the
optimization level variables in target_defaults.

BUG=388949
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287454

[DaleCurtis, 2014-08-04 18:58:17]

https://codereview.chromium.org/437163006/diff/1/base/allocator/allocator.gyp
File base/allocator/allocator.gyp (right):

https://codereview.chromium.org/437163006/diff/1/base/allocator/allocator.gyp...
base/allocator/allocator.gyp:12: # In addition to the above reasons, /Od
optimization won't remove symbols
While true, this comment is probably not relevant to base/. Up to you if you
want to keep it.

https://codereview.chromium.org/437163006/diff/1/base/allocator/allocator.gyp...
base/allocator/allocator.gyp:20: # Setting the optimizations to 'speed' or to
'max' results in a lot of
I'm not sure you want this setting, it's only needed for ffmpeg eccentricities. 
Ditto on the comment.

[willchan no longer on Chromium, 2014-08-04 19:05:22]

I addressed your comments and also ditched Mac since I didn't want to add a
comment explaining that it's not strictly necessary (we don't use TCMalloc on
Mac).

https://codereview.chromium.org/437163006/diff/1/base/allocator/allocator.gyp
File base/allocator/allocator.gyp (right):

https://codereview.chromium.org/437163006/diff/1/base/allocator/allocator.gyp...
base/allocator/allocator.gyp:12: # In addition to the above reasons, /Od
optimization won't remove symbols
On 2014/08/04 18:58:17, DaleCurtis wrote:
> While true, this comment is probably not relevant to base/. Up to you if you
> want to keep it.

Done.

https://codereview.chromium.org/437163006/diff/1/base/allocator/allocator.gyp...
base/allocator/allocator.gyp:20: # Setting the optimizations to 'speed' or to
'max' results in a lot of
On 2014/08/04 18:58:17, DaleCurtis wrote:
> I'm not sure you want this setting, it's only needed for ffmpeg
eccentricities. 
> Ditto on the comment.

Done.

[Nico, 2014-08-04 19:14:38]

Do you have a link to the experiment results on linux somewhere?

lgtm

[willchan no longer on Chromium, 2014-08-04 19:17:32]

See the bug thread.

[willchan no longer on Chromium, 2014-08-04 19:26:44]

The CQ bit was checked by willchan@chromium.org

[DaleCurtis, 2014-08-04 19:27:03]

lgtm

[commit-bot: I haz the power, 2014-08-04 19:28:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/willchan@chromium.org/437163006/20001

[commit-bot: I haz the power, 2014-08-04 21:19:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-05 04:55:36]

Change committed as 287454

[Nico, 2014-08-05 16:57:59]

Looks like this didn't help:
http://build.chromium.org/p/chromium.win/stats/Win7%20Tests%20%28dbg%29%282%29

Do you want to revert?

[willchan no longer on Chromium, 2014-08-05 17:05:50]

Yeah, I updated the bug thread about it. I kinda think it makes sense to
keep it in place. The reason it didn't help is as wfh@ said, our Windows
builders primarily use component builds which don't use TCMalloc. But if we
ever switched back to using TCMalloc on Windows by changing the bot
configurations, then we'd regress debug builder performance. And this kind
of change probably just makes sense in general for a malloc implementation.

I'm open to reverting though if people disagree.


On Tue, Aug 5, 2014 at 9:57 AM, <thakis@chromium.org> wrote:

> Looks like this didn't help:
> http://build.chromium.org/p/chromium.win/stats/Win7%
> 20Tests%20%28dbg%29%282%29
>
> Do you want to revert?
>
> https://codereview.chromium.org/437163006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-08-05 17:06:33]

I'm happy with leaving it in, just wanted to hear your thoughts on it.
Thanks!


On Tue, Aug 5, 2014 at 10:05 AM, William Chan (陈智昌) <willchan@chromium.org>
wrote:

> Yeah, I updated the bug thread about it. I kinda think it makes sense to
> keep it in place. The reason it didn't help is as wfh@ said, our Windows
> builders primarily use component builds which don't use TCMalloc. But if we
> ever switched back to using TCMalloc on Windows by changing the bot
> configurations, then we'd regress debug builder performance. And this kind
> of change probably just makes sense in general for a malloc implementation.
>
> I'm open to reverting though if people disagree.
>
>
> On Tue, Aug 5, 2014 at 9:57 AM, <thakis@chromium.org> wrote:
>
>> Looks like this didn't help:
>> http://build.chromium.org/p/chromium.win/stats/Win7%
>> 20Tests%20%28dbg%29%282%29
>>
>> Do you want to revert?
>>
>> https://codereview.chromium.org/437163006/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.