Inefficient code generated for GetPathForVectorIcon and GetPathForVectorIconAt1xScale |
||
Issue description
The biggest and sixth biggest functions (by bytes of machine code) in chrome.dll on Windows are GetPathForVectorIcon* functions. There sizes and names are:
195980: struct gfx::PathElement const * __cdecl gfx::GetPathForVectorIcon(enum gfx::VectorIconId)
50468 :struct gfx::PathElement const * __cdecl gfx::GetPathForVectorIconAt1xScale(enum gfx::VectorIconId)
Looking at the assembly language shows why - the arrays are being constructed at run-time. This also means that they are all creating per-process private data instead of constant data. Typical machine code looks like this:
const PathElement* GetPathForVectorIcon(VectorIconId id) {
...
0FBCC4B0 6A 05 push 5
0FBCC4B2 B9 30 5F 0E 12 mov ecx,offset path (120E5F30h)
0FBCC4B7 E8 8E FF FF FF call gfx::PathElement::PathElement (0FBCC44Ah)+0
...
0FBCC4BC F3 0F 10 0D 80 8F 06 12 movss xmm1,dword ptr [__real@40c00000 (12068F80h)]
0FBCC4C4 B9 34 5F 0E 12 mov ecx,offset path (120E5F30h)+4
0FBCC4C9 E8 8A FF FF FF call gfx::PathElement::PathElement (0FBCC458h)+0
This same pattern repeats for each element in each array - push or load a constant integer or float, load the address of the element, then call the constructor. The runtime cost of this is negligible, but the wasted code space, .reloc entries, and private memory are unfortunate.
Using constexpr guarantees that the arrays will be done as data instead of code, and actually *decreases* the compile time for vector_icons.cc on Windows.
The effects on other platforms has not been measured but using constexpr should not make things any worse.
It looks like this was originally introduced by crrev.com/1219293004 and is related to crbug.com/505953.
Local tests suggest that the total savings for x86 chrome.dll are:
.text: -257152 bytes change
.rdata: 47984 bytes change
.data: -57344 bytes change
.reloc: -43820 bytes change
Total change: -310332 bytes
In addition to the 257,152 bytes of code reduction and 43,820 bytes of relocation reduction this change moves about 50 KB of data to the read-only data section, instead of having it be per-process private data.
,
Jan 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2681632086283869668714416090a3739ff0e879 commit 2681632086283869668714416090a3739ff0e879 Author: brucedawson <brucedawson@chromium.org> Date: Wed Jan 11 01:12:43 2017 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} [modify] https://crrev.com/2681632086283869668714416090a3739ff0e879/ash/resources/vector_icons/vector_icons.cc.template [modify] https://crrev.com/2681632086283869668714416090a3739ff0e879/chrome/app/vector_icons/vector_icons.cc.template [modify] https://crrev.com/2681632086283869668714416090a3739ff0e879/ui/gfx/vector_icons/vector_icons.cc.template
,
Jan 12 2017
I verified on canary that the functions no longer show up in the SymbolSort report. |
||
►
Sign in to add a comment |
||
Comment 1 by brucedaw...@chromium.org
, Jan 10 2017The results for chrome_child.dll are almost identical. Official build results may be slightly different, but not significantly given that the assembly code listed above came from an official canary build: chrome_child.dll .text: -257120 bytes change .rdata: 48288 bytes change .data: -57280 bytes change .reloc: -43904 bytes change Total change: -310016 bytes