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

Issue 625351 link

Starred by 3 users

Issue metadata

Status: Started
Owner:
Last visit > 30 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Feature
cwp



Sign in to add a comment

Speed up cc::RollingTimeDeltaHistory

Project Member Reported by chongjiang@chromium.org, Jul 2 2016

Issue description

CWP data (internal) shows ~0.1% of all cycles are spent in cc::RollingTimeDeltaHistory::InsertSample, and ~0.01% of all cycles in cc::RollingTimeDeltaHistory::Percentile. Using a single deque as a circular buffer trades faster InsertSample for slower Percentile.

Benchmarking with max_sizes of 60 and 1000 (AFAICT, the only two values currently in use) show InsertSample taking < 0.1x as long as before, and Percentile taking < 3x as long as before.

Given the current distribution, we expect cc::RollingTimeDeltaHistory to fall from ~0.1% to ~0.03% with this change.
 
Summary: Speed up cc::RollingTimeDeltaHistory (was: Use a single deque for cc::RollingTimeDeltaHistory)
Removed implementation detail from the title.

After discussion with dhsharp@, evaluated using a vector to eliminate fragmentation and use less memory. Compared with using a deque, this has slower InsertSample but faster Percentile, with similar total runtime (given the current usage ratio).
https://codereview.chromium.org/2127003002/ for the vector implementation.
Did you want the CL reviewed? It hasn't been published yet, not sure if thats intentional.
Thanks for catching that, I did not realize I needed to hit publish.
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 13 2016

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

commit 9d71ccf7550bfaed0cd64cd168b55471fdfc8330
Author: chongjiang <chongjiang@chromium.org>
Date: Wed Jul 13 02:16:34 2016

Switch cc::RollingTimeDeltaHistory to use a vector

BUG=625351
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review-Url: https://codereview.chromium.org/2127003002
Cr-Commit-Position: refs/heads/master@{#404899}

[modify] https://crrev.com/9d71ccf7550bfaed0cd64cd168b55471fdfc8330/cc/base/rolling_time_delta_history.cc
[modify] https://crrev.com/9d71ccf7550bfaed0cd64cd168b55471fdfc8330/cc/base/rolling_time_delta_history.h

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 3 2016

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

commit 6da29e4e93597849c947a0d998f1ff672993cdb0
Author: chongjiang <chongjiang@chromium.org>
Date: Wed Aug 03 01:05:54 2016

Revert of Switch cc::RollingTimeDeltaHistory to use a vector (patchset #2 id:40001 of https://codereview.chromium.org/2127003002/ )

Reason for revert:
Performance regression: https://bugs.chromium.org/p/chromium/issues/detail?id=629349

Original issue's description:
> Switch cc::RollingTimeDeltaHistory to use a vector
>
> BUG=625351
> CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
>
> Committed: https://crrev.com/9d71ccf7550bfaed0cd64cd168b55471fdfc8330
> Cr-Commit-Position: refs/heads/master@{#404899}

TBR=dhsharp@chromium.org,danakj@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=625351

Review-Url: https://codereview.chromium.org/2161113004
Cr-Commit-Position: refs/heads/master@{#409412}

[modify] https://crrev.com/6da29e4e93597849c947a0d998f1ff672993cdb0/cc/base/rolling_time_delta_history.cc
[modify] https://crrev.com/6da29e4e93597849c947a0d998f1ff672993cdb0/cc/base/rolling_time_delta_history.h

Components: Internals

Sign in to add a comment