New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 718888 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

0.2% regression in sizes at 469566:469566

Project Member Reported by kraynov@chromium.org, May 5 2017

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=718888

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


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

linux
Owner: shend@chromium.org

Comment 3 by shend@chromium.org, May 8 2017

Cc: agrieve@chromium.org
Hi agrieve, sorry to trouble you again. I downloaded the build files from the perf bot before and after my patch. I then ran `supersize diff` and found that there's indeed a 200KB increase in the .text section. However, the diff file is so large that it's difficult to actually find what the cause of error is. Do you know how I can narrow down the cause of the issue? I can't understand how my patch could cause such a big increase.

I've attached the relevant files.
after.size
4.5 MB Download
before.size
4.5 MB Download
diff.txt
2.6 MB View Download
Project Member

Comment 4 by bugdroid1@chromium.org, May 8 2017

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

commit a0fc01a3bcb06cac6e10e6eaf2930b1a0fdb73cc
Author: shend <shend@chromium.org>
Date: Mon May 08 07:11:50 2017

Move nested classes in ComputedStyleBase outside.

Currently, groups such as StyleSurroundData are stored as nested classes
in ComputedStyleBase. It is suspected that this is causing an increase
in binary size. In an atttempt to restore the binary size to its
original value, we move the nested classes outside of ComputedStyleBase.
If this succeeds, we will put these classes in a "detail" namespace to
discourage their use externally.

Diff of generated files:
https://gist.github.com/darrnshn/7d475a02af6f5d57b4a1622721111cba/revisions

BUG= 718888 

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

[modify] https://crrev.com/a0fc01a3bcb06cac6e10e6eaf2930b1a0fdb73cc/third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl

Looks like #4 didn't help at all, but your next one completely fixed it:

"Generate StyleVisualData in ComputedStyleBase." - 60bc5e0281871949b67b8d7d3a2f0bc3db72f98c
https://chromeperf.appspot.com/report?sid=cafe352ff6c97c6e9e7e3c33635cf61c3f7b5dbc15fad921c88d1c9e9c0a87e0&rev=469939


As for looking at the supersize diff, I've taken some examples from your diff and noted them on  bug 717550 . There's certainly more that we could do to reduce noise in the linux diffs through more name normalization and clustering, but as of now I don't have any advice for getting more out of the diff :/

Comment 6 by shend@chromium.org, May 8 2017

Status: Fixed (was: Untriaged)
Thanks agrieve, yeah I'm surprised that just landing more patches fixes the issue :/ I suspect it's some crazy LTO thing happening (which may explain the noisy diff?).

Closing as it is now fixed.
Project Member

Comment 7 by bugdroid1@chromium.org, May 9 2017

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

commit 73b8f7ed8d1105af3d39563f30a2771aee24923f
Author: shend <shend@chromium.org>
Date: Mon May 08 23:59:59 2017

Revert of Move nested classes in ComputedStyleBase outside. (patchset #1 id:1 of https://codereview.chromium.org/2869613002/ )

Reason for revert:
Did not decrease binary size.

Original issue's description:
> Move nested classes in ComputedStyleBase outside.
>
> Currently, groups such as StyleSurroundData are stored as nested classes
> in ComputedStyleBase. It is suspected that this is causing an increase
> in binary size. In an atttempt to restore the binary size to its
> original value, we move the nested classes outside of ComputedStyleBase.
> If this succeeds, we will put these classes in a "detail" namespace to
> discourage their use externally.
>
> Diff of generated files:
> https://gist.github.com/darrnshn/7d475a02af6f5d57b4a1622721111cba/revisions
>
> BUG= 718888 
>
> Review-Url: https://codereview.chromium.org/2869613002
> Cr-Commit-Position: refs/heads/master@{#469911}
> Committed: https://chromium.googlesource.com/chromium/src/+/a0fc01a3bcb06cac6e10e6eaf2930b1a0fdb73cc

TBR=nainar@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 718888 

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

[modify] https://crrev.com/73b8f7ed8d1105af3d39563f30a2771aee24923f/third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl

Labels: -binary-size Performance-Size

Sign in to add a comment