All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=631416

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgupaXgggM


Bot(s) for this bug's original alert(s):

mac

Looks like also the Linux bot got a +400k regression
https://chromeperf.appspot.com/report?sid=226fd8a08e874204633c91071401d6b3d58cf7e868c5e08bf417d471bb2ffd5c&rev=407511

A bit of background: The CL in question converts a JSON features file to produce compiled code, rather than parsing the entire json file at runtime in each process.

I'm sure this is mine, since the CL adds a bunch of generated code.  However, it should also considerably improve browser startup time and the time it takes to initialize any new process (including new renderer processes, of which there are a bunch).  I'm still waiting for data to trickle in, but the performance impact should be significant (local testing indicated a 10x improvement).  Additionally, I see the binary size increasing, but this patch also removes many of the json files from the resources, so the pak file size significantly shrinks (-100k).  This doesn't entirely offset the size difference, but does mitigate it.

I'll post data findings here when they're available, but my inclination is that this will be worth keeping.  Is there a threshold for allowable size increases?

+tapted, Mac sizes owner, and laforge

+kerz@ the desktop & release owner.

My suspicion would be that the JSON files would likely compress down better, so even losing those might not help distribution size.

400+k is a rather large regression, the performance improvements would need to be pretty substantial to justify it.  

How substantial?  Right now in the 95th %ile, this can take up to 700+ms during the critical path in startup and also any new renderer process.  I'm optimistically hoping to trim that down by an order of magnitude (though again, no hard data yet, since this just landed yesterday).  Is that in the right ballpark?

Also worth noting is that right now, if the wrong bit of these json files gets corrupted (say, a comma), we fail to parse and crash chrome.  While this obviously doesn't solve the corruption issue, it does make everything much more resilient (the only bit that's affected is the bit that's corrupted).

I'm probably not needed since this isn't Mac specific. But I have a tip from the sidelines.

IMO 400k of generated code is a lot. I had a quick look at base_feature_provider.h, json_feature_provider.h and feature_compiler.py. I suspect one cause is polymorphism, and it doesn't look like you need it.

Every generated class will have a full vtable, and even though we compile with -fno-rtti, the gcc and clang ABIs still allocate space for rtti info (filled with nulls). MSVC is more efficient about this, which could explain why it's ~250k there rather than ~400k.

But after a cursory look, it seems as though BaseFeatureProvider is the only thing that implements the FeatureProvider interface. JSONFeatureProvider and the generated providers just override the destructor.

So I'd ditch all the classes. Just have `BaseFeatureProvider`. Perhaps give it JSONFeatureProvider |factory_| member which is null for non-JSON stuff [but it looks like |factory| is only used in the constructor, so the member might not be needed?]. Then just generate code that populates the map -- preferably with static array of constructor arguments rather than a series of 

std::unique_ptr<..> foo(new Foo(..))
foo->push_back(..)
map["bar"] = std::move(foo);


(and, big disclaimer: I only had a quick look, there might be a big piece I'm missing that makes this infeasible).

Related:  issue 631915 .  dcheng@, think we should merge 631915 into this one, or do you want to keep them both since 631915 is a little more specific?

We can dupe either way, I don't mind. In any case, I already have a CL to significantly reduce the size impact of this.

The size hit is primarily from the APIFeatureProvider, etc constructors, not the vtables. A lot of code from STL gets expanded inline. The way the generated code is structured ends up implicitly constructing a lot of std::string and std::vector instances. Even though the values are efficiently moved out, the std::string and std::vector destructor don't end up compiling to less code. std::string and std::vector's constructor and destructor are quite heavy: 5.7MB of Chrome's text section is taken up by inlined basic_string destructors.

I landed https://chromium.googlesource.com/chromium/src.git/+/d8b8acff3d2d57e66b833a5632e3a993eb405277, which helped quite a bit with Linux / Mac. However, the improvement on Windows was much smaller. Unfortunately, I won't be able to easily investigate the Windows side of things in the near future: hopefully someone else can take a look and try to figure out what's going on with the Windows codegen.

+grt, owner of Windows sizes, for any ideas on #12

 Issue 631915  has been merged into this issue.

Bruce and Scott: do either of you have tools at hand to see where the extra code size is coming from?

I don't have a good magic differ between revisions. There's tools/win/sizeviewer and the microsoft example dia2dump which can both be useful.

I'll take a look and see if I can see any obvious cause for the size. (Ugh, official :()

My experience with low-level tools is pretty limited, but I'm happy to help out if I can.  Daniel, do we know what the impact is after your patch landed?  The graphs (https://chromeperf.appspot.com/report?sid=9401ee2b031ddfa3bcde52eae6cf602d840150a0088911fd315fa1d50aa3e5fa) indicate that there were a few subsequent patches increasing size significantly, so it's a bit hard to pinpoint an exact number.

(And of course, *some* size increase is expected, since this did land a good chunk of code, but I wouldn't have expected it to be terribly significant for "only" ~5000 lines.)

Linux x64:
  net increase of 437400 bytes with the initial patch
  net decrease of 291000 bytes with the followup patch

There are probably further optimizations, but this is a good start.

Windows chrome.dll:
  net increase: 237600 bytes
  net decrease: 74300 bytes

Windows chrome_child.dll:
  Value: 236100 bytes
  Value: 74200 bytes

(I don't have Mac numbers anymore, because the increase has already scrolled off the LHS. I'm also a bit fuzzy on the difference between chrome.dll and chrome_child.dll: I thought they built in different things, but I guess not always?)

One theory I already tried:
- Reuse the same unique_ptr objects by only creating the ones we need, reusing them with reset, and static_casting to the appropriate pointer type to call any subclass methods. This seemed to actually increase binary size on Windows.

Theories I wanted to test:
- Try using raw pointers: maybe the compilers have to emit a bunch of code to unwind the temporary unique_ptrs?
- ???

But it's probably more useful to look at what MSVC is emitting and trying to make things more efficient from there.

Oh, another theory is that MSVC is being more clever and inlining the code even after it's been out-of-lined.

Okay, so net increases are around 146k for linux, 160k for win chrome.dll, and 160k for win chrome_child.dll?

(I'm assuming that "net decrease" actually means "decrease from followup" rather than "decrease compared to before initial patch").

Do we have an acceptable range?  Obviously as small as possible is the goal, but it'd be good to know if we're in the ballpark yet.

For the record, data [1] are showing that this patch *is* helping in performance, reducing feature initialization by 100+ ms in the 75th %ile and  a whopping 600+ms in the 95%ile.  That's initialization that happens in the browser process on startup (in the critical path) and in every single renderer process created on desktop.

[1] https://uma.googleplex.com/p/chrome/timeline_v2/?sid=d6b5ae8617d52f78326dc0c807d30f68

chrome is browser, chrome_child is renderer, et al. Unfortunately because this code is in common/, it's duplicated into both dlls.

So that means it's not that much different between platforms then -- effectively 146 vs. 163 as far as possible win-only differences go.

(I'm still waiting on win official)

Ah, OK: I guess MSVC was doing a better job to begin with, so the changes just didn't have as much impact there. I can just keep approaching it with an official Linux build next week then: +160KB from 5K LoC doesn't seem entirely unreasonable, though if we can find low-hanging fruit, that would also be nice.

I think an interesting thing to figure out would be how this compares to the "average" function: is this emitting more bytes of code per line of code than the usual function in Chromium? If we have some idea of what that number is, then we could probably use that as a rough benchmark for good enough.

I've used customized versions of dia2dump, but my customizations focus on the data segment, which is much easier to deal with.

When working on a recent sizes regression in gn I tried comparing the set of functions between gyp and gn builds but while this worked well on chrome_elf.dll, it failed for chrome.dll because the differences were too great and I couldn't tease out any useful patterns.

I tried the sizes.py script but I also couldn't anything useful from it, which surprises me.

Re: #22, why do you say MSVC was doing a better job to start with?


I did some tests on regular Release Win to iterate more quickly. For the body of APIFeatureList::APIFeatureList:

r407511: 0x1b588
r407511 + leiz's change cherry picked: 0x11943
r407511 + leiz's change cherry picked, plus https://codereview.chromium.org/2193693003 (raw ptr instead of unique_ptr): 0xa54a

So, a similar percentage reduction in normal release, at least. Attached the 3 dumps for reference. I haven't waited for official builds at those revs, so may or may not be relevant.

Deleted: 407511.txt 9.9 MB

407511.txt 9.9 MB View Download

Deleted: 407511-plus-leiz.txt 5.9 MB

407511-plus-leiz.txt 5.9 MB View Download

Deleted: 407511-plus-more.txt 2.9 MB

407511-plus-more.txt 2.9 MB View Download

Because the size increase was smaller on Windows: the effect is just magnified since it's compiled into two binaries.

What is leiz's change?

I have no idea! Sorry, I somehow managed to swap "leiz" in for "dcheng". :/ I'm going crazy.

But it's d8b8acff3d2d57e66b833a5632e3a993eb405277. I'll stick to hashes from now on.

Official Win builds now.

For body of APIFeatureProvider::APIFeatureProvider():
r407511: 112007 bytes
cherry-pick dcheng's patch: 72002 bytes
cherry-pick dcheng's patch + https://codereview.chromium.org/2193693003: 42313 bytes

child_child.dll binary size:
r407511: 48,372,736
cherry-pick dcheng's patch: 48,295,424 (-77312)
cherry-pick dcheng's patch + https://codereview.chromium.org/2193693003: 48,240,640 (-54784 more)

dumpbin /all /disasm of official binaries here: https://drive.google.com/open?id=0BxgGCUbbk6gYR1BSbXJIWmU0SnM in case someone wants to dig more.

I'm not going to wait for chrome.dll officials (3 times), but I'd expect the reductions to be almost identical for that binary, so the savings is 2x that.

So if https://codereview.chromium.org/2193693003 looks correct-ish to someone and not too uglifying I guess it's worthwhile.

(Sorry again about the name mixup, I feel like an idiot)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3

commit 2026cea7fa1b2146d7113dc31f2a2969d9ec58e3
Author: scottmg <scottmg@chromium.org>
Date: Fri Jul 29 22:40:21 2016

Further reduction of feature_compiler.py generated code

Use raw pointers instead of unique pointers to reduce size. See linked
bug for size details.

BUG= 631416 

Review-Url: https://codereview.chromium.org/2193693003
Cr-Commit-Position: refs/heads/master@{#408788}

[modify] https://crrev.com/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3/extensions/common/features/base_feature_provider.cc
[modify] https://crrev.com/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3/extensions/common/features/base_feature_provider.h
[modify] https://crrev.com/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3/extensions/common/features/complex_feature.cc
[modify] https://crrev.com/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3/extensions/common/features/complex_feature.h
[modify] https://crrev.com/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3/extensions/common/features/complex_feature_unittest.cc
[modify] https://crrev.com/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3/extensions/common/features/json_feature_provider.cc
[modify] https://crrev.com/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3/extensions/common/features/simple_feature_unittest.cc
[modify] https://crrev.com/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3/tools/json_schema_compiler/feature_compiler.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/132a21ca3dedb4b2942737a3e947c772c81743aa

commit 132a21ca3dedb4b2942737a3e947c772c81743aa
Author: tapted <tapted@chromium.org>
Date: Mon Aug 01 06:16:08 2016

Revert of Further reduction of feature_compiler.py generated code (patchset #5 id:80001 of https://codereview.chromium.org/2193693003/ )

Reason for revert:
Suspect for widespread Win7 NaCl failures starting with
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/builds/55503

Wasn't r408782, r408787 or r408781

Original issue's description:
> Further reduction of feature_compiler.py generated code
>
> Use raw pointers instead of unique pointers to reduce size. See linked
> bug for size details.
>
> BUG= 631416 
>
> Committed: https://crrev.com/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3
> Cr-Commit-Position: refs/heads/master@{#408788}

TBR=rdevlin.cronin@chromium.org,dcheng@chromium.org,scottmg@chromium.org
NOTRY=true
NOTREECHECKS=true
BUG= 631416 

Review-Url: https://codereview.chromium.org/2201653003
Cr-Commit-Position: refs/heads/master@{#408917}

[modify] https://crrev.com/132a21ca3dedb4b2942737a3e947c772c81743aa/extensions/common/features/base_feature_provider.cc
[modify] https://crrev.com/132a21ca3dedb4b2942737a3e947c772c81743aa/extensions/common/features/base_feature_provider.h
[modify] https://crrev.com/132a21ca3dedb4b2942737a3e947c772c81743aa/extensions/common/features/complex_feature.cc
[modify] https://crrev.com/132a21ca3dedb4b2942737a3e947c772c81743aa/extensions/common/features/complex_feature.h
[modify] https://crrev.com/132a21ca3dedb4b2942737a3e947c772c81743aa/extensions/common/features/complex_feature_unittest.cc
[modify] https://crrev.com/132a21ca3dedb4b2942737a3e947c772c81743aa/extensions/common/features/json_feature_provider.cc
[modify] https://crrev.com/132a21ca3dedb4b2942737a3e947c772c81743aa/extensions/common/features/simple_feature_unittest.cc
[modify] https://crrev.com/132a21ca3dedb4b2942737a3e947c772c81743aa/tools/json_schema_compiler/feature_compiler.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c18de0470b513e16ce2c283ff89452cb0c974d7

commit 0c18de0470b513e16ce2c283ff89452cb0c974d7
Author: tapted <tapted@chromium.org>
Date: Mon Aug 01 07:09:34 2016

Reland of Further reduction of feature_compiler.py generated code (patchset #1 id:1 of https://codereview.chromium.org/2201653003/ )

Reason for revert:
Not this either

Original issue's description:
> Revert of Further reduction of feature_compiler.py generated code (patchset #5 id:80001 of https://codereview.chromium.org/2193693003/ )
>
> Reason for revert:
> Suspect for widespread Win7 NaCl failures starting with
> https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/builds/55503
>
> Wasn't r408782, r408787 or r408781
>
> Original issue's description:
> > Further reduction of feature_compiler.py generated code
> >
> > Use raw pointers instead of unique pointers to reduce size. See linked
> > bug for size details.
> >
> > BUG= 631416 
> >
> > Committed: https://crrev.com/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3
> > Cr-Commit-Position: refs/heads/master@{#408788}
>
> TBR=rdevlin.cronin@chromium.org,dcheng@chromium.org,scottmg@chromium.org
> NOTRY=true
> NOTREECHECKS=true
> BUG= 631416 
>
> Committed: https://crrev.com/132a21ca3dedb4b2942737a3e947c772c81743aa
> Cr-Commit-Position: refs/heads/master@{#408917}

TBR=rdevlin.cronin@chromium.org,dcheng@chromium.org,scottmg@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 631416 

Review-Url: https://codereview.chromium.org/2199683002
Cr-Commit-Position: refs/heads/master@{#408922}

[modify] https://crrev.com/0c18de0470b513e16ce2c283ff89452cb0c974d7/extensions/common/features/base_feature_provider.cc
[modify] https://crrev.com/0c18de0470b513e16ce2c283ff89452cb0c974d7/extensions/common/features/base_feature_provider.h
[modify] https://crrev.com/0c18de0470b513e16ce2c283ff89452cb0c974d7/extensions/common/features/complex_feature.cc
[modify] https://crrev.com/0c18de0470b513e16ce2c283ff89452cb0c974d7/extensions/common/features/complex_feature.h
[modify] https://crrev.com/0c18de0470b513e16ce2c283ff89452cb0c974d7/extensions/common/features/complex_feature_unittest.cc
[modify] https://crrev.com/0c18de0470b513e16ce2c283ff89452cb0c974d7/extensions/common/features/json_feature_provider.cc
[modify] https://crrev.com/0c18de0470b513e16ce2c283ff89452cb0c974d7/extensions/common/features/simple_feature_unittest.cc
[modify] https://crrev.com/0c18de0470b513e16ce2c283ff89452cb0c974d7/tools/json_schema_compiler/feature_compiler.py

I think we've reduced this enough that it's back in the acceptable range.  If anyone disagrees, please reopen.