Remove /Zc:sizedDealloc- hack

Remove /Zc:sizedDealloc- hack

Early versions of VS 2015 had an implementation of sizedDealloc that
caused link errors. Later versions improved it so it can handle Chrome
now, although its results are still not guaranteed, and it can end up
pulling in object files that would otherwise not be referenced.

Testing with the settings below I find that this change (reenabling sized
dealloc) costs us a total of about 190 KB, mostly in code size:

is_chrome_branded = true
is_component_build = false
is_debug = false
target_cpu = "x86"
is_official_build = true
full_wpo_on_official = true

chrome.exe
     .text:  1568 bytes change
    .rdata:   112 bytes change
    .reloc:    28 bytes change
Total change:  1708 bytes

chrome.dll
     .text: 84384 bytes change
    .rdata:  4832 bytes change
    .reloc:  4212 bytes change
Total change: 93428 bytes

chrome_child.dll
     .text: 88580 bytes change
    .rdata:  3232 bytes change
     .data:    -4 bytes change
    .reloc:  3216 bytes change
Total change: 95024 bytes

BUG=526851, 594758

[brucedawson, 2016-12-05 01:19:25]

The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 01:19:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 03:13:20]

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

[commit-bot: I haz the power, 2016-12-05 03:13:21]

Dry run: This issue passed the CQ dry run.

[brucedawson, 2016-12-05 19:29:48]

Description was changed from

==========
Remove /Zc:sizedDealloc- hack

Early versions of VS 2015 had an implementation of sizedDealloc that
caused link errors. Later versions improved it so it can handle Chrome
now.

BUG=526851
==========

to

==========
Remove /Zc:sizedDealloc- hack

Early versions of VS 2015 had an implementation of sizedDealloc that
caused link errors. Later versions improved it so it can handle Chrome
now, although its results are still not guaranteed, and it can end up
pulling in object files that would otherwise not be referenced.

BUG=526851
==========

[brucedawson, 2016-12-05 19:31:29]

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

[brucedawson, 2016-12-05 19:31:29]

I guessed that this change had the potential to affect binary size so I ran some
size tests. I used these settings:

is_chrome_branded = true
is_component_build = false
is_debug = false
target_cpu = "x86"

Under these circumstances I saw a non-trivial change in the size of our main
binaries:

chrome.exe
     .text:  1824 bytes change
    .rdata:    80 bytes change
    .reloc:    12 bytes change
Total change:  1916 bytes

chrome.dll
     .text: 108704 bytes change
    .rdata:  4592 bytes change
    .reloc:  3624 bytes change
Total change: 116920 bytes

chrome_child.dll
     .text: 117328 bytes change
    .rdata:  2688 bytes change
    .reloc:  2788 bytes change
Total change: 122804 bytes

I'm not clear on the benefits of sizedDealloc so I'm inclined to close this CL
and change the comment to explain that we are now using sizedDealloc because it
saves us ~250 KB.

Thoughts?

[Nico, 2016-12-12 21:56:29]

Wow nice, lgtm.

In crbug.com/594758 I said "Also, if we placement new anywhere so that the
dynamic size of a type doesn't match its static type, things won't work. We
might do this in v8." -- but bots seem happy, so I think this is worth a try.
The savings look pretty good. We should give this a try on e.g. Android too!

[brucedawson, 2016-12-12 21:58:19]

On 2016/12/12 21:56:29, Nico wrote:
> Wow nice, lgtm.
> 
> In crbug.com/594758 I said "Also, if we placement new anywhere so that the
> dynamic size of a type doesn't match its static type, things won't work. We
> might do this in v8." -- but bots seem happy, so I think this is worth a try.
> The savings look pretty good. We should give this a try on e.g. Android too!

My message was confusing. What I meant was that /Zc:sizedDealloc- is saving us
about 250 KB. That is, committing this change will *cost* us about 250 KB.

Oops. I'll update the description to make this clearer.

[brucedawson, 2016-12-12 21:59:20]

Description was changed from

==========
Remove /Zc:sizedDealloc- hack

Early versions of VS 2015 had an implementation of sizedDealloc that
caused link errors. Later versions improved it so it can handle Chrome
now, although its results are still not guaranteed, and it can end up
pulling in object files that would otherwise not be referenced.

BUG=526851
==========

to

==========
Remove /Zc:sizedDealloc- hack

Early versions of VS 2015 had an implementation of sizedDealloc that
caused link errors. Later versions improved it so it can handle Chrome
now, although its results are still not guaranteed, and it can end up
pulling in object files that would otherwise not be referenced.

Testing with the settings below I find that this change (reenabling sized
dealloc) costs us a total of about 250 KB, mostly in code size:

is_chrome_branded = true
is_component_build = false
is_debug = false
target_cpu = "x86"

chrome.exe
     .text:  1824 bytes change
    .rdata:    80 bytes change
    .reloc:    12 bytes change
Total change:  1916 bytes

chrome.dll
     .text: 108704 bytes change
    .rdata:  4592 bytes change
    .reloc:  3624 bytes change
Total change: 116920 bytes

chrome_child.dll
     .text: 117328 bytes change
    .rdata:  2688 bytes change
    .reloc:  2788 bytes change
Total change: 122804 bytes

BUG=526851
==========

[Nico, 2016-12-12 22:01:36]

Huh, that's surprising, I would've expected that to help...I guess it helps more
with perf than with binary size though. Are the 250kB all for pushing another
arg to operator delete?

[Nico, 2016-12-12 22:07:05]

(Hans points out that is_official_build / LTCG might change the numbers here,
and due to crbug.com/673025 even then that wouldn't really test what we actually
ship :-(

[brucedawson, 2016-12-12 22:44:28]

On 2016/12/12 22:07:05, Nico wrote:
> (Hans points out that is_official_build / LTCG might change the numbers here,
> and due to crbug.com/673025 even then that wouldn't really test what we
actually
> ship :-(

I'll test with is_official_build and full_wpo_on_official and see if that makes
any difference.

It would be that there is some genuine code-size cost to supporting
sizedDealloc. Or, it could be that the wacky implementation is pulling in a
randomly chosen object file (again) which is causing the size regression. I'll
compare the set of global variables to see if that gives any indication of which
it is.

[brucedawson, 2016-12-13 07:14:08]

Description was changed from

==========
Remove /Zc:sizedDealloc- hack

Early versions of VS 2015 had an implementation of sizedDealloc that
caused link errors. Later versions improved it so it can handle Chrome
now, although its results are still not guaranteed, and it can end up
pulling in object files that would otherwise not be referenced.

Testing with the settings below I find that this change (reenabling sized
dealloc) costs us a total of about 250 KB, mostly in code size:

is_chrome_branded = true
is_component_build = false
is_debug = false
target_cpu = "x86"

chrome.exe
     .text:  1824 bytes change
    .rdata:    80 bytes change
    .reloc:    12 bytes change
Total change:  1916 bytes

chrome.dll
     .text: 108704 bytes change
    .rdata:  4592 bytes change
    .reloc:  3624 bytes change
Total change: 116920 bytes

chrome_child.dll
     .text: 117328 bytes change
    .rdata:  2688 bytes change
    .reloc:  2788 bytes change
Total change: 122804 bytes

BUG=526851
==========

to

==========
Remove /Zc:sizedDealloc- hack

Early versions of VS 2015 had an implementation of sizedDealloc that
caused link errors. Later versions improved it so it can handle Chrome
now, although its results are still not guaranteed, and it can end up
pulling in object files that would otherwise not be referenced.

Testing with the settings below I find that this change (reenabling sized
dealloc) costs us a total of about 190 KB, mostly in code size:

is_chrome_branded = true
is_component_build = false
is_debug = false
target_cpu = "x86"
is_official_build = true
full_wpo_on_official = true

chrome.exe
     .text:  1568 bytes change
    .rdata:   112 bytes change
    .reloc:    28 bytes change
Total change:  1708 bytes

chrome.dll
     .text: 84384 bytes change
    .rdata:  4832 bytes change
    .reloc:  4212 bytes change
Total change: 93428 bytes

chrome_child.dll
     .text: 88580 bytes change
    .rdata:  3232 bytes change
     .data:    -4 bytes change
    .reloc:  3216 bytes change
Total change: 95024 bytes

BUG=526851
==========

[brucedawson, 2016-12-13 07:16:09]

I retested with a full official build - everything but PGO. I think this
represents reality.

Turning off the hack (re-enabling sizeddealloc) costs 190 (metric) KB, mostly in
code size. I checked for changes in large global variables and saw none. So, it
is probably something about sizedDealloc that makes the code larger. Odd.

If we need/want the feature then we should do it, but it's unfortunate that it
moves things in the wrong direction.

[brucedawson, 2016-12-13 22:13:25]

Description was changed from

==========
Remove /Zc:sizedDealloc- hack

Early versions of VS 2015 had an implementation of sizedDealloc that
caused link errors. Later versions improved it so it can handle Chrome
now, although its results are still not guaranteed, and it can end up
pulling in object files that would otherwise not be referenced.

Testing with the settings below I find that this change (reenabling sized
dealloc) costs us a total of about 190 KB, mostly in code size:

is_chrome_branded = true
is_component_build = false
is_debug = false
target_cpu = "x86"
is_official_build = true
full_wpo_on_official = true

chrome.exe
     .text:  1568 bytes change
    .rdata:   112 bytes change
    .reloc:    28 bytes change
Total change:  1708 bytes

chrome.dll
     .text: 84384 bytes change
    .rdata:  4832 bytes change
    .reloc:  4212 bytes change
Total change: 93428 bytes

chrome_child.dll
     .text: 88580 bytes change
    .rdata:  3232 bytes change
     .data:    -4 bytes change
    .reloc:  3216 bytes change
Total change: 95024 bytes

BUG=526851
==========

to

==========
Remove /Zc:sizedDealloc- hack

Early versions of VS 2015 had an implementation of sizedDealloc that
caused link errors. Later versions improved it so it can handle Chrome
now, although its results are still not guaranteed, and it can end up
pulling in object files that would otherwise not be referenced.

Testing with the settings below I find that this change (reenabling sized
dealloc) costs us a total of about 190 KB, mostly in code size:

is_chrome_branded = true
is_component_build = false
is_debug = false
target_cpu = "x86"
is_official_build = true
full_wpo_on_official = true

chrome.exe
     .text:  1568 bytes change
    .rdata:   112 bytes change
    .reloc:    28 bytes change
Total change:  1708 bytes

chrome.dll
     .text: 84384 bytes change
    .rdata:  4832 bytes change
    .reloc:  4212 bytes change
Total change: 93428 bytes

chrome_child.dll
     .text: 88580 bytes change
    .rdata:  3232 bytes change
     .data:    -4 bytes change
    .reloc:  3216 bytes change
Total change: 95024 bytes

BUG=526851,594758
==========

[brucedawson, 2017-01-11 22:39:29]

The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 22:41:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[brucedawson, 2017-01-11 22:49:07]

The CQ bit was unchecked by brucedawson@chromium.org