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

Issue 686148 link

Starred by 1 user

Issue metadata

Status: Duplicate
Merged: issue 759519
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



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

Project Member Reported by pras...@chromium.org, Jan 27 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jan 27 2017

Cc: alexclarke@chromium.org
Owner: alexclarke@chromium.org

=== 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!
Cc: skyos...@chromium.org dcheng@chromium.org
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!

Comment 5 by dcheng@chromium.org, Jan 27 2017

Happy to revisit my patch, but do you mind explaining what's happening? =)
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.
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.
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.

Comment 9 by dcheng@chromium.org, 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.
Labels: Performance-Memory
Status: Assigned (was: Untriaged)
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.
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. 

Project Member

Comment 15 by 42576172...@developer.gserviceaccount.com, Jul 28 2017

Cc: ricea@chromium.org
Owner: ricea@chromium.org

=== 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
Cc: benhenry@chromium.org
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?
Project Member

Comment 17 by 42576172...@developer.gserviceaccount.com, 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
Feel free to re-purpose, but make sure to update the summary and maybe comment #0.
Summary: 3.27% regression in memory:chrome:all_processes:reported_by_chrome:v8:effective_size_avg/load_news/load_news_hackernews (was: 1.3%-16.2% regression in memory.top_10_mobile_stress at 446356:446407)

Comment 20 by ricea@chromium.org, Aug 21 2017

Components: Blink>Network>StreamsAPI

Comment 21 by ricea@chromium.org, Aug 28 2017

Mergedinto: 759519
Status: Duplicate (was: Assigned)

Sign in to add a comment