Optimize GetPathForVectorIcon*, save ~290 KB on disk

Optimize GetPathForVectorIcon*, save ~290 KB on disk

VC++ was initializing all of the static PathElement arrays in
GetPathForVectorIcon and GetPathForVectorIconAt1xScale at runtime, which
meant that these were the largest and sixth largest functions in
chrome.dll, and similarly in chrome_child.dll. Tagging the arrays and
types as constexpr shrinks these functions down to nothing, saves about
~50 KB of per-process private data, and actually helps them compile
slightly faster.

The approximate section size changes are:
       .text: -164224 bytes change
      .rdata:   51984 bytes change
       .data:  -54976 bytes change
      .reloc:  -23204 bytes change

These gains apply both to chrome.dll and chrome_child.dll.

BUG=679539
Review-Url: https://codereview.chromium.org/2620653004
Cr-Commit-Position: refs/heads/master@{#442751}
Committed: https://chromium.googlesource.com/chromium/src/+/2681632086283869668714416090a3739ff0e879

[brucedawson, 2017-01-10 00:53:10]

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

[brucedawson, 2017-01-10 00:53:31]

Description was changed from

==========
Optimize GetPathForVectorIcon*, save ~500 KB on disk

VC++ was initializing all of the static PathElement arrays in
GetPathForVectorIcon and GetPathForVectorIconAt1xScale at runtime, which
meant that these were the largest and sixth largest functions in
chrome.dll, and similarly in chrome_child.dll. Tagging the arrays and
types as constexpr shrinks these functions down to nothing, saves about
~50 KB of per-process private data, and actually helps them compile
slightly faster.

The approximate section size changes for chrome.dll and chrome_child.dll
are:
       .text: -257152 bytes change
      .rdata:   47984 bytes change
       .data:  -57344 bytes change
      .reloc:  -43820 bytes change

BUG=679539
==========

to

==========
Optimize GetPathForVectorIcon*, save ~500 KB on disk

VC++ was initializing all of the static PathElement arrays in
GetPathForVectorIcon and GetPathForVectorIconAt1xScale at runtime, which
meant that these were the largest and sixth largest functions in
chrome.dll, and similarly in chrome_child.dll. Tagging the arrays and
types as constexpr shrinks these functions down to nothing, saves about
~50 KB of per-process private data, and actually helps them compile
slightly faster.

The approximate section size changes for chrome.dll and chrome_child.dll
are:
       .text: -257152 bytes change
      .rdata:   47984 bytes change
       .data:  -57344 bytes change
      .reloc:  -43820 bytes change

BUG=679539
==========

[brucedawson, 2017-01-10 00:53:31]

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

[commit-bot: I haz the power, 2017-01-10 00:54:05]

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, 2017-01-10 02:56:03]

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

[commit-bot: I haz the power, 2017-01-10 02:56:04]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[brucedawson, 2017-01-10 05:06:51]

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

[commit-bot: I haz the power, 2017-01-10 05:07:20]

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, 2017-01-10 05:16:04]

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

[commit-bot: I haz the power, 2017-01-10 05:16:05]

Dry run: This issue passed the CQ dry run.

[brucedawson, 2017-01-10 05:28:16]

Description was changed from

==========
Optimize GetPathForVectorIcon*, save ~500 KB on disk

VC++ was initializing all of the static PathElement arrays in
GetPathForVectorIcon and GetPathForVectorIconAt1xScale at runtime, which
meant that these were the largest and sixth largest functions in
chrome.dll, and similarly in chrome_child.dll. Tagging the arrays and
types as constexpr shrinks these functions down to nothing, saves about
~50 KB of per-process private data, and actually helps them compile
slightly faster.

The approximate section size changes for chrome.dll and chrome_child.dll
are:
       .text: -257152 bytes change
      .rdata:   47984 bytes change
       .data:  -57344 bytes change
      .reloc:  -43820 bytes change

BUG=679539
==========

to

==========
Optimize GetPathForVectorIcon*, save ~500 KB on disk

VC++ was initializing all of the static PathElement arrays in
GetPathForVectorIcon and GetPathForVectorIconAt1xScale at runtime, which
meant that these were the largest and sixth largest functions in
chrome.dll, and similarly in chrome_child.dll. Tagging the arrays and
types as constexpr shrinks these functions down to nothing, saves about
~50 KB of per-process private data, and actually helps them compile
slightly faster.

The approximate section size changes are:
       .text: -257152 bytes change
      .rdata:   47984 bytes change
       .data:  -57344 bytes change
      .reloc:  -43820 bytes change

These gains apply both to chrome.dll and chrome_child.dll.

BUG=679539
==========

[brucedawson, 2017-01-10 05:28:22]

The CQ bit was checked by brucedawson@chromium.org

[brucedawson, 2017-01-10 05:28:26]

The CQ bit was unchecked by brucedawson@chromium.org

[brucedawson, 2017-01-10 05:28:42]

PTAL - simple change for some nice wins.

[Evan Stade, 2017-01-10 17:30:26]

geeze, thanks.

https://codereview.chromium.org/2620653004/diff/1/ash/resources/vector_icons/...
File ash/resources/vector_icons/vector_icons.cc.template (right):

https://codereview.chromium.org/2620653004/diff/1/ash/resources/vector_icons/...
ash/resources/vector_icons/vector_icons.cc.template:14: static constexpr
gfx::PathElement path_name[] = {__VA_ARGS__};
is this part actually necessary? I ask because I didn't find it necessary when
fixing a related problem with static initializers.

There are a few vector_icons.cc.template files out there ---
chrome/app/vector_icons/ and ash/resources/vector_icons/ both have them. Should
those be updated as well?

https://codereview.chromium.org/2620653004/diff/1/ui/gfx/vector_icon_types.h
File ui/gfx/vector_icon_types.h (right):

https://codereview.chromium.org/2620653004/diff/1/ui/gfx/vector_icon_types.h#...
ui/gfx/vector_icon_types.h:67: constexpr PathElement(CommandType value) :
type(value) {}
I believe your tree is slightly out of date :)

[brucedawson, 2017-01-10 18:31:54]

https://codereview.chromium.org/2620653004/diff/1/ash/resources/vector_icons/...
File ash/resources/vector_icons/vector_icons.cc.template (right):

https://codereview.chromium.org/2620653004/diff/1/ash/resources/vector_icons/...
ash/resources/vector_icons/vector_icons.cc.template:14: static constexpr
gfx::PathElement path_name[] = {__VA_ARGS__};
On 2017/01/10 17:30:26, Evan Stade wrote:
> is this part actually necessary? I ask because I didn't find it necessary when
> fixing a related problem with static initializers.

I meant to ask about that because changes to this file did not affect the build
at all. I decided to add constexpr because it was so obvious and simple and it
seemed better than leaving it as-is. Alternately the file could be deleted if
not used. Thoughts?

> There are a few vector_icons.cc.template files out there ---
> chrome/app/vector_icons/ and ash/resources/vector_icons/ both have them.
Should
> those be updated as well?

Yes. I'll look for them.

https://codereview.chromium.org/2620653004/diff/1/ui/gfx/vector_icon_types.h
File ui/gfx/vector_icon_types.h (right):

https://codereview.chromium.org/2620653004/diff/1/ui/gfx/vector_icon_types.h#...
ui/gfx/vector_icon_types.h:67: constexpr PathElement(CommandType value) :
type(value) {}
On 2017/01/10 17:30:26, Evan Stade wrote:
> I believe your tree is slightly out of date :)

What bizarre timing that I made this change the same day that you landed the
same thing.

constexpr all the things!

[brucedawson, 2017-01-10 18:39:04]

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

[commit-bot: I haz the power, 2017-01-10 18:39:35]

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, 2017-01-10 19:45:08]

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

[commit-bot: I haz the power, 2017-01-10 19:45:09]

Dry run: This issue passed the CQ dry run.

[brucedawson, 2017-01-10 21:19:44]

Description was changed from

==========
Optimize GetPathForVectorIcon*, save ~500 KB on disk

VC++ was initializing all of the static PathElement arrays in
GetPathForVectorIcon and GetPathForVectorIconAt1xScale at runtime, which
meant that these were the largest and sixth largest functions in
chrome.dll, and similarly in chrome_child.dll. Tagging the arrays and
types as constexpr shrinks these functions down to nothing, saves about
~50 KB of per-process private data, and actually helps them compile
slightly faster.

The approximate section size changes are:
       .text: -257152 bytes change
      .rdata:   47984 bytes change
       .data:  -57344 bytes change
      .reloc:  -43820 bytes change

These gains apply both to chrome.dll and chrome_child.dll.

BUG=679539
==========

to

==========
Optimize GetPathForVectorIcon*, save ~290 KB on disk

VC++ was initializing all of the static PathElement arrays in
GetPathForVectorIcon and GetPathForVectorIconAt1xScale at runtime, which
meant that these were the largest and sixth largest functions in
chrome.dll, and similarly in chrome_child.dll. Tagging the arrays and
types as constexpr shrinks these functions down to nothing, saves about
~50 KB of per-process private data, and actually helps them compile
slightly faster.

The approximate section size changes are:
       .text: -164224 bytes change
      .rdata:   51984 bytes change
       .data:  -54976 bytes change
      .reloc:  -23204 bytes change

These gains apply both to chrome.dll and chrome_child.dll.

BUG=679539
==========

[brucedawson, 2017-01-10 21:26:43]

I updated the change to pull to latest and to add constexpr to all three
applicable places.

The change to ash\resources\vector_icons\vector_icons.cc.template seems to have
no effect for some reason, but I'm inclined to make the change - as long as the
file exists it should be up-to-date. Thoughts?

I updated the listed savings in the description. The savings are slightly
smaller now, but still worthwhile.

PTAL!

[Evan Stade, 2017-01-11 00:26:22]

lgtm

https://codereview.chromium.org/2620653004/diff/1/ash/resources/vector_icons/...
File ash/resources/vector_icons/vector_icons.cc.template (right):

https://codereview.chromium.org/2620653004/diff/1/ash/resources/vector_icons/...
ash/resources/vector_icons/vector_icons.cc.template:14: static constexpr
gfx::PathElement path_name[] = {__VA_ARGS__};
On 2017/01/10 18:31:54, brucedawson wrote:
> On 2017/01/10 17:30:26, Evan Stade wrote:
> > is this part actually necessary? I ask because I didn't find it necessary
when
> > fixing a related problem with static initializers.
> 
> I meant to ask about that because changes to this file did not affect the
build
> at all. I decided to add constexpr because it was so obvious and simple and it
> seemed better than leaving it as-is. Alternately the file could be deleted if
> not used. Thoughts?
> 
> > There are a few vector_icons.cc.template files out there ---
> > chrome/app/vector_icons/ and ash/resources/vector_icons/ both have them.
> Should
> > those be updated as well?
> 
> Yes. I'll look for them.

the file is used, but I suspect you're not building Chrome OS (ash is only built
on ChromeOS these days). What I meant was that constexpr on this declaration
didn't seem to affect the static initializers; only the constexpr on the
PathElement ctors was necessary, so I wasn't sure if it was necessary for the
improvements listed in this CL.

[brucedawson, 2017-01-11 01:05:55]

https://codereview.chromium.org/2620653004/diff/1/ash/resources/vector_icons/...
File ash/resources/vector_icons/vector_icons.cc.template (right):

https://codereview.chromium.org/2620653004/diff/1/ash/resources/vector_icons/...
ash/resources/vector_icons/vector_icons.cc.template:14: static constexpr
gfx::PathElement path_name[] = {__VA_ARGS__};
Ah. Got it.

For VC++ putting constexpr on the array definitions is crucial. Without that
nothing changes.

Clearly this sort of thing is compiler dependent, but the great thing about
constexpr on the arrays is that it *guarantees* compile-time construction.
Anything less than that (including constexpr on the PathElement constructors)
is, at best, a hint.

So, constexpr on the PathElement constructors is necessary but not (necessarily)
sufficient.

It's good that the ChromeOS compiler figured this out without needing as much
constexpr.

There are some VC++ limitations that stop us from using constexpr as much as I
would like but I've put in a strongly worded feature request which they are
considering.

[brucedawson, 2017-01-11 01:06:50]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2017-01-11 01:07:29]

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, 2017-01-11 01:12:47]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484096810539360,
"parent_rev": "f8bee1d41368c1a6f66901fdf65dbdb49e002127", "commit_rev":
"2681632086283869668714416090a3739ff0e879"}

[commit-bot: I haz the power, 2017-01-11 01:13:59]

Description was changed from

==========
Optimize GetPathForVectorIcon*, save ~290 KB on disk

VC++ was initializing all of the static PathElement arrays in
GetPathForVectorIcon and GetPathForVectorIconAt1xScale at runtime, which
meant that these were the largest and sixth largest functions in
chrome.dll, and similarly in chrome_child.dll. Tagging the arrays and
types as constexpr shrinks these functions down to nothing, saves about
~50 KB of per-process private data, and actually helps them compile
slightly faster.

The approximate section size changes are:
       .text: -164224 bytes change
      .rdata:   51984 bytes change
       .data:  -54976 bytes change
      .reloc:  -23204 bytes change

These gains apply both to chrome.dll and chrome_child.dll.

BUG=679539
==========

to

==========
Optimize GetPathForVectorIcon*, save ~290 KB on disk

VC++ was initializing all of the static PathElement arrays in
GetPathForVectorIcon and GetPathForVectorIconAt1xScale at runtime, which
meant that these were the largest and sixth largest functions in
chrome.dll, and similarly in chrome_child.dll. Tagging the arrays and
types as constexpr shrinks these functions down to nothing, saves about
~50 KB of per-process private data, and actually helps them compile
slightly faster.

The approximate section size changes are:
       .text: -164224 bytes change
      .rdata:   51984 bytes change
       .data:  -54976 bytes change
      .reloc:  -23204 bytes change

These gains apply both to chrome.dll and chrome_child.dll.

BUG=679539

Review-Url: https://codereview.chromium.org/2620653004
Cr-Commit-Position: refs/heads/master@{#442751}
Committed:
https://chromium.googlesource.com/chromium/src/+/2681632086283869668714416090...
==========

[commit-bot: I haz the power, 2017-01-11 01:14:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2681632086283869668714416090...

[Wez, 2017-02-07 02:31:17]

wez@chromium.org changed reviewers:
+ grt@chromium.org, tdanderson@chromium.org, wez@chromium.org

[Wez, 2017-02-07 02:31:18]

Hallo grt@chromium.org, tdanderson@chromium.org!
Due to a depot_tools patch which mistakenly removed the OWNERS check for
non-source files (see crbug.com/684270), the following files landed in this CL
and need a retrospective review from you:
	ash/resources/vector_icons/vector_icons.cc.template
	chrome/app/vector_icons/vector_icons.cc.template
	ui/gfx/vector_icons/vector_icons.cc.template
Thanks,
Wez

[Wez, 2017-02-07 02:34:20]

On 2017/02/07 02:31:18, Wez wrote:
> Hallo mailto:grt@chromium.org, mailto:tdanderson@chromium.org!
> Due to a depot_tools patch which mistakenly removed the OWNERS check for
> non-source files (see crbug.com/684270), the following files landed in this CL
> and need a retrospective review from you:
> 	ash/resources/vector_icons/vector_icons.cc.template
> 	chrome/app/vector_icons/vector_icons.cc.template
> 	ui/gfx/vector_icons/vector_icons.cc.template
> Thanks,
> Wez

Hmmm, please ignore this; for some reason this CL got flagged despite estade@
being OWNER. :(

[Wez, 2017-02-07 02:34:40]

Description was changed from

==========
Optimize GetPathForVectorIcon*, save ~290 KB on disk

VC++ was initializing all of the static PathElement arrays in
GetPathForVectorIcon and GetPathForVectorIconAt1xScale at runtime, which
meant that these were the largest and sixth largest functions in
chrome.dll, and similarly in chrome_child.dll. Tagging the arrays and
types as constexpr shrinks these functions down to nothing, saves about
~50 KB of per-process private data, and actually helps them compile
slightly faster.

The approximate section size changes are:
       .text: -164224 bytes change
      .rdata:   51984 bytes change
       .data:  -54976 bytes change
      .reloc:  -23204 bytes change

These gains apply both to chrome.dll and chrome_child.dll.

BUG=679539

Review-Url: https://codereview.chromium.org/2620653004
Cr-Commit-Position: refs/heads/master@{#442751}
Committed:
https://chromium.googlesource.com/chromium/src/+/2681632086283869668714416090...
==========

to

==========
Optimize GetPathForVectorIcon*, save ~290 KB on disk

VC++ was initializing all of the static PathElement arrays in
GetPathForVectorIcon and GetPathForVectorIconAt1xScale at runtime, which
meant that these were the largest and sixth largest functions in
chrome.dll, and similarly in chrome_child.dll. Tagging the arrays and
types as constexpr shrinks these functions down to nothing, saves about
~50 KB of per-process private data, and actually helps them compile
slightly faster.

The approximate section size changes are:
       .text: -164224 bytes change
      .rdata:   51984 bytes change
       .data:  -54976 bytes change
      .reloc:  -23204 bytes change

These gains apply both to chrome.dll and chrome_child.dll.

BUG=679539

Review-Url: https://codereview.chromium.org/2620653004
Cr-Commit-Position: refs/heads/master@{#442751}
Committed:
https://chromium.googlesource.com/chromium/src/+/2681632086283869668714416090...
==========

[Wez, 2017-02-07 02:34:40]

wez@chromium.org changed reviewers:
- grt@chromium.org, tdanderson@chromium.org, wez@chromium.org

[Wez, 2017-02-07 05:54:23]

wez@chromium.org changed reviewers:
+ grt@chromium.org, tdanderson@chromium.org, wez@chromium.org

[Wez, 2017-02-07 05:54:24]

OK, re-instating grt@ and tdanderson@ as reviewers, for the ash/resources and
chrome/app files.

[grt (UTC plus 2), 2017-02-07 07:46:15]

chrome/app lgtm

https://codereview.chromium.org/2620653004/diff/40001/chrome/app/vector_icons...
File chrome/app/vector_icons/vector_icons.cc.template (right):

https://codereview.chromium.org/2620653004/diff/40001/chrome/app/vector_icons...
chrome/app/vector_icons/vector_icons.cc.template:17: const gfx::VectorIcon
icon_name = { path_name , path_name_1x };
why not here as well? it may not have the same wondrous gains as the macro
above, but it seems equally valid for this to be static constexpr (unless i'm
missing something).

[brucedawson, 2017-02-21 17:52:12]

https://codereview.chromium.org/2620653004/diff/40001/chrome/app/vector_icons...
File chrome/app/vector_icons/vector_icons.cc.template (right):

https://codereview.chromium.org/2620653004/diff/40001/chrome/app/vector_icons...
chrome/app/vector_icons/vector_icons.cc.template:17: const gfx::VectorIcon
icon_name = { path_name , path_name_1x };
On 2017/02/07 07:46:15, grt (UTC plus 1) wrote:
> why not here as well? it may not have the same wondrous gains as the macro
> above, but it seems equally valid for this to be static constexpr (unless i'm
> missing something).

The only reason it wasn't added was because it wasn't needed. Agreed that it
would be valid and appropriate. At some point we should do an aggressive pass
over Chrome's source code to add constexpr to thousands of places. If VC++ 2017
gets a constexpr feature that I'm hoping for then this process can be pushed
much further.

So, we could add constexpr here (the static is actually unneeded as const and
constexpr both imply static - I should have removed the static from line 14) but
I'm inclined to treat doing so so as no higher priority than the other
~thousands of const globals.

[tdanderson, 2017-03-21 17:45:24]

On 2017/02/21 17:52:12, brucedawson wrote:
>
https://codereview.chromium.org/2620653004/diff/40001/chrome/app/vector_icons...
> File chrome/app/vector_icons/vector_icons.cc.template (right):
> 
>
https://codereview.chromium.org/2620653004/diff/40001/chrome/app/vector_icons...
> chrome/app/vector_icons/vector_icons.cc.template:17: const gfx::VectorIcon
> icon_name = { path_name , path_name_1x };
> On 2017/02/07 07:46:15, grt (UTC plus 1) wrote:
> > why not here as well? it may not have the same wondrous gains as the macro
> > above, but it seems equally valid for this to be static constexpr (unless
i'm
> > missing something).
> 
> The only reason it wasn't added was because it wasn't needed. Agreed that it
> would be valid and appropriate. At some point we should do an aggressive pass
> over Chrome's source code to add constexpr to thousands of places. If VC++
2017
> gets a constexpr feature that I'm hoping for then this process can be pushed
> much further.
> 
> So, we could add constexpr here (the static is actually unneeded as const and
> constexpr both imply static - I should have removed the static from line 14)
but
> I'm inclined to treat doing so so as no higher priority than the other
> ~thousands of const globals.

ash/resources LGTM