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

Issue 807606 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 651354



Sign in to add a comment

Add a metric for time-to-scheduled on ItemParallelJob::Task

Project Member Reported by gab@chromium.org, Jan 31 2018

Issue description

We suspect that other background work may preempt (or even prevent altogether) important background tasks from being scheduled.

Add a metric to test this hypothesis
 
Cc: rmcilroy@chromium.org
It'd be great if this could apply to all important background tasks (e.g., compile as well as GC).

Comment 2 by hpayer@google.com, Feb 1 2018

Yes. This should be independent of V8.

Comment 3 by gab@chromium.org, Feb 1 2018

We already have latency tasks for TaskScheduler (TaskScheduler.TaskLatencyMicroseconds.Renderer.*). What I'd like to add is multiple ones (suffixed) that are specific to each V8 background task (because one will go up and the other will go down once we prioritize then properly).

As such I don't see how in can be independent of V8?
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/954146a5cf923cfa0462f22d4525fbb81b96d761

commit 954146a5cf923cfa0462f22d4525fbb81b96d761
Author: Gabriel Charette <gab@chromium.org>
Date: Thu Feb 01 11:55:42 2018

Make TimeTicks::Now() high-resolution whenever possible with low-latency.

It was already always high-resolution on POSIX but was never high
resolution on Windows. Windows does support low latency high-resolution
timers for the majority of our user base.

TimeTicks::HighResolutionNow() was only explicitly requested in testing
frameworks. As such I left the call in place but made it DCHECK that
it's running on a Windows machine on which high-resolution clocks are
used. This confirms that none of our test fleet has regressed with this
change (the previous HighResolutionNow() used to be slightly more
aggressive and also do it in a few configurations where we now fallback
to low-resolution now).

This implementation was copied as-is (modulo minor v8 API
compatibility tweaks). These implementations were the same in the
past but had diverged when, sadly, the same bug was fixed separately
years apart, in Chromium and V8:
chromium: https://codereview.chromium.org/1284053004 + https://codereview.chromium.org/2393953003
v8: https://codereview.chromium.org/1304873011

This is a prerequisite to add metrics around parallel task execution
(low-resolution clocks are useless at that level, but we also don't want
to incur high-latency clocks on machines that can't afford it cheaply).

Bug:  chromium:807606 
Change-Id: Id18e7be895d8431ebd0e565a1bdf358fe7838489
Reviewed-on: https://chromium-review.googlesource.com/897485
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51027}
[modify] https://crrev.com/954146a5cf923cfa0462f22d4525fbb81b96d761/src/base/platform/time.cc
[modify] https://crrev.com/954146a5cf923cfa0462f22d4525fbb81b96d761/src/base/platform/time.h
[modify] https://crrev.com/954146a5cf923cfa0462f22d4525fbb81b96d761/test/unittests/base/platform/time-unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/db73d446b91ee8aaa52ddf4d347e29ba9aa0d4f5

commit db73d446b91ee8aaa52ddf4d347e29ba9aa0d4f5
Author: Gabriel Charette <gab@chromium.org>
Date: Fri Feb 02 15:38:55 2018

Bring Time(Delta)::Min/Max() and related helpers to V8.

Copied as-is modulo compile tweaks from Chromium's base.

Copied tests highlighting existing overflow issues with V8's impl...

TimeDelta::Max() will initially be used in V8 to flag events that
never triggered in a TimedHistogram.

Also constexpr'ed a few things while I was in there, it's harmless
at worst and helps a little at best.
Ideally would constexpr all the Time*::From*() methods like in
Chromium but that has inlining implications and I don't know the
impact that could have on V8.

Bug:  chromium:807606 
Change-Id: If5aa92759d985be070e12af4dd20f0159169048b
Reviewed-on: https://chromium-review.googlesource.com/899342
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51073}
[modify] https://crrev.com/db73d446b91ee8aaa52ddf4d347e29ba9aa0d4f5/src/base/platform/time.cc
[modify] https://crrev.com/db73d446b91ee8aaa52ddf4d347e29ba9aa0d4f5/src/base/platform/time.h
[modify] https://crrev.com/db73d446b91ee8aaa52ddf4d347e29ba9aa0d4f5/test/unittests/base/platform/time-unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Feb 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/85b8daed6357e6d0e104912f1c6454b91ce4abef

commit 85b8daed6357e6d0e104912f1c6454b91ce4abef
Author: Gabriel Charette <gab@chromium.org>
Date: Tue Feb 06 21:53:49 2018

Add V8.GC.ParallelTaskLatencyMicroSeconds metric.

It will record the time-to-schedule-after-job-start for different
task types to try to highlight use cases where contention might
be a problem (and show improvements to it later).

Also introducing AsyncTimedHistogram to support this use case whose
reported timings go beyond a single scope (i.e. the async version of
ScopedTimedHistogram).

Bug:  chromium:807606 
Change-Id: Ib4d581fa8b001723dfe8c91102280e9608b4fabb
Reviewed-on: https://chromium-review.googlesource.com/899365
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51131}
[modify] https://crrev.com/85b8daed6357e6d0e104912f1c6454b91ce4abef/src/counters.cc
[modify] https://crrev.com/85b8daed6357e6d0e104912f1c6454b91ce4abef/src/counters.h
[modify] https://crrev.com/85b8daed6357e6d0e104912f1c6454b91ce4abef/src/heap/heap.cc
[modify] https://crrev.com/85b8daed6357e6d0e104912f1c6454b91ce4abef/src/heap/item-parallel-job.h
[modify] https://crrev.com/85b8daed6357e6d0e104912f1c6454b91ce4abef/src/heap/mark-compact.cc
[modify] https://crrev.com/85b8daed6357e6d0e104912f1c6454b91ce4abef/test/unittests/heap/item-parallel-job-unittest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Feb 8 2018

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

commit 8beba4645043b30dbf87c1f9301e6acc802d719c
Author: Gabriel Charette <gab@chromium.org>
Date: Thu Feb 08 11:40:22 2018

Add V8.GC.ParallelTaskLatencyMicroSeconds to histograms.xml.

Matching V8 CL @ https://chromium-review.googlesource.com/c/v8/v8/+/899365

R=isherman@chromium.org

Bug:  807606 
Change-Id: I9b1b5d9677234ef58a97df1d7606e16582a53b00
Reviewed-on: https://chromium-review.googlesource.com/905602
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535357}
[modify] https://crrev.com/8beba4645043b30dbf87c1f9301e6acc802d719c/tools/metrics/histograms/histograms.xml

Comment 8 by gab@chromium.org, Feb 21 2018

Status: Fixed (was: Started)
Implemented the metric, the 99th'ile is quite high @ 9ms. This indicates that tasks in the renderer might tend to steal the worker pool too much with parallel work. Finer grain work items and priorities on each one would help here.

Also, going after the long tail of slow interactions isn't really worth it until we've split all of V8's performance stats into foreground/background (we explicitly don't want to improve the long tail of GCs for invisible tabs).

https://uma.googleplex.com/p/chrome/histograms/?endDate=latest&dayCount=7&histograms=V8.GC.ParallelTaskLatencyMicroSeconds&fixupData=true&showMax=true&filters=platform%2Ceq%2CA%2Cchannel%2Ceq%2C1%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial

Sign in to add a comment