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

Issue 645876 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

3.1%-7.7% regression in speedometer at 417843:417850

Project Member Reported by nikolaos@chromium.org, Sep 12 2016

Issue description

See the link to graphs below.
 
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Sep 12 2016

Cc: f...@opera.com
Owner: f...@opera.com

=== Auto-CCing suspected CL author fs@opera.com ===

Hi fs@opera.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 : Revert of Make canceling Timers fast. (patchset #10 id:180001 of https://codereview.chromium.org/2319053004/ )
Author  : fs
Commit description:
  
Reason for revert:
Wreaks havoc on the ASAN bots:

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN

STDERR: =================================================================
STDERR: ==4==ERROR: AddressSanitizer: use-after-poison on address 0x7ed9a5616190 at pc 0x00000768b0da bp 0x7fff4bbca630 sp 0x7fff4bbca628
STDERR: READ of size 8 at 0x7ed9a5616190 thread T0 (content_shell)
STDERR:     #0 0x768b0d9 in operator-> third_party/WebKit/Source/wtf/RefPtr.h:68:50
STDERR:     #1 0x768b0d9 in revokeAll third_party/WebKit/Source/wtf/WeakPtr.h:146:0
STDERR:     #2 0x768b344 in ?? third_party/WebKit/Source/platform/Timer.cpp:124:22
STDERR:     #3 0x47a4461 in Run base/callback.h:56:12
STDERR:     #4 0x47a4461 in RunTask base/debug/task_annotator.cc:54:0
STDERR:     #5 0x79e2381 in ProcessTaskFromWorkQueue third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:316:19
...
STDERR: AddressSanitizer can not describe address in more detail (wild memory access suspected).
STDERR: SUMMARY: AddressSanitizer: use-after-poison

Appears to be accessing a "user-poison" area, so maybe a timer in something that was swept? (Wild guess.)

Original issue's description:
> Make canceling Timers fast.
>
> base::Closure recently got an IsCancelled method. Taking advantage of
> that the scheduler can short circuit a bunch of logic for cancelled
> tasks and avoid running them and the rest of the task selection
> machinery.
>
> On the new TimerPerfTest benchmark this makes running 10000 cancelled
> tasks aprox 50x - 60x faster (measured on Android and Linux).
>
> Note this patch reverts many of the changes made in
> https://codereview.chromium.org/2258713004 in favor of
> WeakPtr based cancellation as favored by the base owners.
>
> BUG= 605718 ,  638542 
>
> Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1
> Cr-Commit-Position: refs/heads/master@{#417621}

TBR=jochen@chromium.org,haraken@chromium.org,skyostil@chromium.org,alexclarke@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 605718 ,  638542 

Review-Url: https://codereview.chromium.org/2326313003
Cr-Commit-Position: refs/heads/master@{#417846}
Commit  : bee7b64b4772d14c9cafc00f60629a89e8f5bf81
Date    : Sat Sep 10 21:59:53 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@417845  2556.77  46.5827  5  good
chromium@417846  2720.83  66.683   5  bad    <--
chromium@417847  2707.56  68.0172  5  bad

Bisect job ran on: android_nexus7_perf_bisect
Bug ID: 645876

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests speedometer
Test Metric: FlightJS-TodoMVC/FlightJS-TodoMVC
Relative Change: 5.90%
Score: 99.5

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus7_perf_bisect/builds/3304
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9001740813842344432


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

| 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 f...@opera.com, Sep 12 2016

Owner: alexclarke@chromium.org
That commit is a revert. I expect these tests will recover when the CL is relanded. Assigning to (original) CL author.
Oh interesting. I did hope some of the metrics might improve with the patch but didn't find any till now.  I'm hopeful the ASAN issue can be fixed and the patch relanded.
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 12 2016

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

commit 77f183aaf924ea7742df562455db8579607224e0
Author: alexclarke <alexclarke@chromium.org>
Date: Mon Sep 12 13:59:45 2016

Make canceling Timers fast.

base::Closure recently got an IsCancelled method. Taking advantage of
that the scheduler can short circuit a bunch of logic for cancelled
tasks and avoid running them and the rest of the task selection
machinery.

On the new TimerPerfTest benchmark this makes running 10000 cancelled
tasks aprox 50x - 60x faster (measured on Android and Linux).

Note this patch reverts many of the changes made in
https://codereview.chromium.org/2258713004 in favor of
WeakPtr based cancellation as favored by the base owners.

NOTE it's possible this might break
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN

I was not able to reproduce the ASAN failure locally however
I suspect it's due to the Timer getting swept away which
should be fixed by the change in the latest patchset.

BUG= 605718 ,  638542 ,  645876 

Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1
Review-Url: https://codereview.chromium.org/2319053004
Cr-Original-Commit-Position: refs/heads/master@{#417621}
Cr-Commit-Position: refs/heads/master@{#417931}

[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/Timer.cpp
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/Timer.h
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_perftest.cc
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/base/time_domain.cc
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/base/work_queue.cc
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/base/work_queue.h
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/base/work_queue_sets_unittest.cc
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/base/work_queue_unittest.cc
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/child/web_task_runner_impl.cc
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/child/web_task_runner_impl.h
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/test/fake_web_task_runner.cc
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/platform/scheduler/test/fake_web_task_runner.h
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/web/BUILD.gn
[add] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/Source/web/tests/TimerPerfTest.cpp
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/public/platform/WebTaskRunner.h
[modify] https://crrev.com/77f183aaf924ea7742df562455db8579607224e0/third_party/WebKit/public/platform/scheduler/base/task_queue.h

Status: Fixed (was: Assigned)
I re-landed the patch and the perf regression has gone away.

Sign in to add a comment