Issue metadata
Sign in to add a comment
|
1% regression in system_health.memory_desktop at 407360:407361 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jul 25 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/9006152659410304352
,
Jul 25 2016
=== 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!
,
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.
,
Aug 5 2016
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.
,
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.
,
Aug 5 2016
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
,
Aug 5 2016
Yeah I checked that but there are no memory results available, just the ones for times.
,
Aug 8 2016
#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.
,
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
,
Aug 17 2016
#10: Thanks!
,
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?
,
Aug 30 2016
,
Aug 30 2016
,
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
,
Sep 15 2016
@petercermak: was it fixed by the above commit?
,
Sep 16 2016
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.
,
Oct 3 2016
Thanks for the fix!
,
Oct 3 2016
Welcome! |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by petrcermak@chromium.org
, Jul 25 2016