New issue
Advanced search Search tips

Issue 631057 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

1% regression in system_health.memory_desktop at 407360:407361

Project Member Reported by petrcermak@chromium.org, Jul 25 2016

Issue description

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

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


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

chromium-rel-win7-dual
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jul 25 2016

Cc: svil...@igalia.com
Owner: svil...@igalia.com

=== Auto-CCing suspected CL author svillar@igalia.com ===

Hi svillar@igalia.com, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : [css-grid] grid-auto-flow|row should take a <track-size>+
Author  : svillar
Commit description:
  
Both properties were traditionally accepting just one <track-size> but
a recent change in the spec allows authors to pass a non-empty list of track
sizes. The first implicit track will use the first track size the second
implicit track will use the second track size and so on. For implicit tracks
before the explicit grid we need to transpose the order, i.e, the first
implicit track before the explicit grid will take the last track size.

Updated parsing, styling and track size resolution to support a list of
track sizes instead of just one. The grid shorthand was also updated as it
can set the value of the two grid-auto-* properties.

BUG= 618970 

Review-Url: https://codereview.chromium.org/2166393002
Cr-Commit-Position: refs/heads/master@{#407361}
Commit  : ef8ba7e30cc8392bb48ad021e59d3aa496c930f1
Date    : Sat Jul 23 09:36:46 2016


===== TESTED REVISIONS =====
Revision         Mean    Std Dev      N  Good?
chromium@407359  781544  0.0          5  good
chromium@407360  781544  0.0          5  good
chromium@407361  789736  1.84069e-10  5  bad    <--

Bisect job ran on: win_perf_bisect
Bug ID: 631057

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_desktop
Test Metric: load_games-memory:chrome:all_processes:reported_by_chrome:partition_alloc:effective_size_avg/load_games_lazors
Relative Change: 1.05%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/6728
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9006152659410304352


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=6683771856748544

| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Tests>AutoBisect.  Thank you!

Comment 4 by svil...@igalia.com, Jul 26 2016

So that change is replacing a couple of GridTrackSize structures in the ComputedStyle object by Vectors, so it indeed increases the memory consumption. What we could do, provided that the default case is just having 1 value inside the Vector is to reserve capacity for 1 one element and let it grow later if needed.
svillar: I don't have the domain-specific knowledge, so I'm unable to judge if a +8 KiB regression is acceptable for this given change. However, if there's a fix that could reduce the regression without other aspects of performance, then I think it's worth it. Could you please give it a try? Let me know if you need help reproducing the numbers.

Comment 6 by svil...@igalia.com, Aug 5 2016

petrcermak: the problem is that it's extremely complicated to use the perf infrastructure for non-gogglers as I could not make it work locally. I tried to send a job with a potential fix to a perf bot and this is what I got

https://codereview.chromium.org/2179963004/

not sure if the green means that something is fixed or not. I totally miss extra information.
If you click through to linux_perf_bisect, you'll see a results.html link that points you here: http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-07-26_11-10-32

Comment 8 by svil...@igalia.com, Aug 5 2016

Yeah I checked that but there are no memory results available, just the ones for times.
#6: I'm really sorry about the situation. Green means that the bot succeeded in generating results (regardless of whether they are better/worse/same).

#8: All the values in http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-07-26_11-10-32 are actually memory results even though they're classified under 'time'. Sorry, results.html are deprecated and are currently being replaced with results2.html.

Please have one more look and let me know if I can be of any assistance.

Comment 10 by svil...@igalia.com, Aug 17 2016

OK I see, the situation with the tools is indeed far from ideal.

I'm currently on holidays. Will check it out next week.h
#10: Thanks!

Comment 12 by svil...@igalia.com, Aug 25 2016

petrcermak@chromium.org: I've uploaded a tentative fix here https://codereview.chromium.org/2264133002/ but there is no way to get an overall result (like 1% regression or 3% improvement) just reading the trybot results. How do you get the figures mentioned in the bug report?

Is there any way you could apply the patch and verify it fixes the issue?
Labels: SystemHealth-Sheriff
Labels: -Performance-Sheriff
Project Member

Comment 15 by bugdroid1@chromium.org, Sep 1 2016

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

commit 5d2f646883db3f2abe8a503c9702698d1e3cfab7
Author: svillar <svillar@igalia.com>
Date: Thu Sep 01 08:59:46 2016

[css-grid] Fix 1% regression on system_health.memory_desktop

Apparently crrev.com/407361 added a 1% memory regression. The only thing
that could be increasing memory consumption is the fact that we now require
a vector to store the auto tracks instead of just a single GridTrack. We can
make the Vector just reserve 1 element of capacity by default.

BUG= 631057 

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

[modify] https://crrev.com/5d2f646883db3f2abe8a503c9702698d1e3cfab7/third_party/WebKit/Source/core/style/ComputedStyle.cpp

Comment 16 by svil...@igalia.com, Sep 15 2016

@petercermak: was it fixed by the above commit?
Labels: Hotlist-SystemHealthBankruptcy
Status: Archived (was: Assigned)
Temporarily declaring bankruptcy on the *desktop* system health benchmark.
The number of alerts became unmanageable and the overall process needs to be improved to make it sustainable.
The alerts have been turned off and I'm archiving the outstanding regressions.
Note: this is just about desktop, the mobile system health stays up. 
Status: Fixed (was: Archived)
svillar: I can see a -4 KiB improvement between r415655 and r415935, so I think it's safe to mark this as fixed.
Thanks for the fix!
Welcome!

Sign in to add a comment