Issue metadata
Sign in to add a comment
|
3.27% regression in memory:chrome:all_processes:reported_by_chrome:v8:effective_size_avg/load_news/load_news_hackernews |
||||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jan 27 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8989291978587117952
,
Jan 27 2017
=== Auto-CCing suspected CL author alexclarke@chromium.org === Hi alexclarke@chromium.org, the bisect results pointed to your CL, please take a look at the results. === BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : alexclarke Commit : eda1d2e08c6769ebdbae66db618a2fae609f170c Date : Thu Jan 26 18:23:44 2017 Subject: Scheduler refactoring to virtually eliminate redundant DoWorks Bisect Details Configuration: android_one_perf_bisect Benchmark : thread_times.key_idle_power_cases Metric : tasks_per_second_total_all/animated-gif.html Change : 14.48% | 5.1189478933 -> 5.86004024829 Revision Result N chromium@446360 5.11895 +- 0.29086 6 good chromium@446377 5.01823 +- 0.197151 6 good chromium@446379 5.08383 +- 0.316464 6 good chromium@446380 5.85056 +- 0.296349 6 bad <-- chromium@446381 5.84264 +- 0.436205 6 bad chromium@446385 5.85054 +- 0.179263 6 bad chromium@446393 5.86004 +- 0.423826 6 bad To Run This Test src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=animated.gif.html thread_times.key_idle_power_cases Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8989291978587117952 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5787374446968832 | 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!
,
Jan 27 2017
That's not unexpected, that patch was written assuming support for cancelled tasks in the base messaggloop. Fortunately +dcheng@ has a patch to do that :) Lets try and land it!
,
Jan 27 2017
Happy to revisit my patch, but do you mind explaining what's happening? =)
,
Jan 30 2017
Sure. Prior to the patch, if there where N distinct base::TimeTicks for requested delayed tasks, the blink scheduler would post N delayed tasks to run TaskQueueManager::DoWork on the underlying messageloop. We did not do anything sensible when delayed tasks where cancelled (and this is really common with javascript timers). Now we only support one outstanding delayed task. Suppose there is already a delayed DoWork scheduled for time t=10 but a new task is posted for time t=1, well we cancel the first delayed task and post one for t=1. Once the t=1 task has been run, we re-post the delayed task. Without support for cancellation in base this metric will regress. Incidentally for non-trivial pages we should get less tasks run even without changes in base because the old DoWork de-duplication didn't work very well. We should probably add some more complex pages to this metric.
,
Jan 31 2017
I should probably add, the reason for landing the DoWork patch was we want to be able to use the scheduler on the cc thread. There is a thread time regression associated with doing so and the DoWork patch goes a long way to fixing that but only if base supports cancellation.
,
Feb 17 2017
Update: I talked to dcheng@ at blinkon and we discussed landing the patch to cancelled tasks in the base messageloop. Probably not super urgent to land that, but it should fix this.
,
Feb 17 2017
As an update from my side, I've rebased the patch and got it working... but it makes one test explode with thread-safety issues in WeakPtrFactory.
,
Mar 11 2017
,
Jul 27 2017
Explictly assigning. A CL you landed tripped one of the speed metrics we measure in the lab. If this is the first time this has happened to one of your CLs, or if it's been a while, please read: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/addressing_performance_regressions.md We're looking for one of the following: 1. Justification via explanation 2. Plan to revert or fix 3. Angry rage throwing of equipment at my head Just be aware that I'm trained in trumpet playing and First Aid and am not afraid to use it. Note: This was a bulk edit message and not very personal.
,
Jul 28 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8972842742611912720
,
Jul 28 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8972842588990518976
,
Jul 28 2017
Lost track of this one, re-visiting the graphs I'm not convinced this patch actually did cause a regression in the first place. The tasks_per_second_total_all/animated-gif.html metric is inconstant across the three android devices. The android-one & android-nexus5X improved before something in the patch range reverted them to the previous mean. The graph for the android-nexus5 doesn't show an improvement before but something in the patch range seemed to regress it but it went away shortly after. The net effect is the metric ended up where they where shortly before. I note in passing that dchang@'s patch did land. There is a good chance the memory regressions are something else in the patch range. It's very tempting as a perf sheriff to triage a bunch of unrelated things in one patch range down to one bug because the UI erroneously encourages that. I've kicked off bisects on those, we'll see what is found.
,
Jul 28 2017
=== Auto-CCing suspected CL author ricea@chromium.org === Hi ricea@chromium.org, the bisect results pointed to your CL, please take a look at the results. === BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : ricea Commit : 9ba4a643d92a0ec3890aa1c9b7f2be349ad2c928 Date : Thu Jan 26 17:11:57 2017 Subject: Implementation of ReadableStream pipeTo and pipeThrough Bisect Details Configuration: android_nexus6_perf_bisect Benchmark : system_health.memory_mobile Metric : memory:chrome:all_processes:reported_by_chrome:v8:effective_size_avg/load_news/load_news_hackernews Change : 3.27% | 1995067.33333 -> 2060344.0 Revision Result N chromium@446310 1995067 +- 215.438 6 good chromium@446341 1995741 +- 255.604 6 good chromium@446356 1995788 +- 0.0 6 good chromium@446357 1995695 +- 323.316 6 good chromium@446358 2066813 +- 215.438 6 bad <-- chromium@446360 2066773 +- 272.509 6 bad chromium@446364 2066655 +- 215.438 6 bad chromium@446371 2060344 +- 0.0 6 bad Please refer to the following doc on diagnosing memory regressions: https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md To Run This Test src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.news.hackernews system_health.memory_mobile More information on addressing performance regressions: http://g.co/ChromePerformanceRegressions Debug information about this bisect: https://chromeperf.appspot.com/buildbucket_job_status/8972842742611912720 For feedback, file a bug with component Speed>Bisection
,
Jul 28 2017
Question for Ben. Looks like I was right and two separate issues are aliased to the same bug. I'd argue the tasks_per_second_total_all/animated-gif.html metric "regression" wasn't one at all based on the graphs and we can ignore it. So should we use this bug now to investigate the memory regression or start a new bug?
,
Jul 28 2017
=== BISECT JOB RESULTS === NO Perf regression found Bisect Details Configuration: android_nexus5_perf_bisect Benchmark : memory.top_10_mobile_stress Metric : memory:chrome:all_processes:reported_by_os:system_memory:native_heap:proportional_resident_size_avg/background/after_https_www_google_co_uk_hl_en_q_science Revision Result N chromium@446356 32373816 +- 2926456 21 good chromium@446408 32096312 +- 2415564 21 bad Please refer to the following doc on diagnosing memory regressions: https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md To Run This Test src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests memory.top_10_mobile_stress More information on addressing performance regressions: http://g.co/ChromePerformanceRegressions Debug information about this bisect: https://chromeperf.appspot.com/buildbucket_job_status/8972842588990518976 For feedback, file a bug with component Speed>Bisection
,
Jul 28 2017
Feel free to re-purpose, but make sure to update the summary and maybe comment #0.
,
Aug 4 2017
,
Aug 21 2017
,
Aug 28 2017
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by pras...@chromium.org
, Jan 27 2017