New issue
Advanced search Search tips

Issue 872123 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 17
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

3.5%-40.8% regression in rendering.mobile at 579862:580150

Project Member Reported by toyoshim@chromium.org, Aug 8

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=872123

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=f14654835a75902483a043d803f25f7bff7594560be6098b6b7ea6df118996b2


Bot(s) for this bug's original alert(s):

Android Nexus5X WebView Perf
Android Nexus6 WebView Perf
😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/121e836fa40000

The value of property "state" is longer than 1048487 bytes.
Cc: smcgruer@chromium.org alex...@chromium.org
Owner: smcgruer@chromium.org
Status: Assigned (was: Untriaged)
📍 Found significant differences after each of 2 commits.
https://pinpoint-dot-chromeperf.appspot.com/job/14a343e2640000

Remove temporary DumpWithoutCrashing to InitRenderFrameProxy() by alexmos@chromium.org
https://chromium.googlesource.com/chromium/src/+/c79afd77ff3e596039645afc16dc806309f8c61c
27.98 → 25.16 (-2.818)

Add full saturate-to-infinity behavior to base::TimeDelta by smcgruer@chromium.org
https://chromium.googlesource.com/chromium/src/+/2b5dfbb0a1d4eee3d5df8ac7bd280488b054c63d
28.37 → 16.9 (-11.47)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
Cc: dcheng@chromium.org
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 16

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

commit b1aaab647bd8905a1a16bee1bbefc8e44929070f
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Thu Aug 16 15:00:46 2018

Revert "Add full saturate-to-infinity behavior to base::TimeDelta"

This reverts commit 2b5dfbb0a1d4eee3d5df8ac7bd280488b054c63d.

Reason for revert: Strong suspect for performance regression (https://pinpoint-dot-chromeperf.appspot.com/job/14a343e2640000), and disagreements on original CL implementation. Reverting until they can be resolved.

Bug:  872123 , 869387

Original change's description:
> Add full saturate-to-infinity behavior to base::TimeDelta
>
> Before this CL TimeDelta had saturation behavior for translating
> from/to other values, but had clamping behavior for its computation.
> That is, invariants like the following did not hold:
>
> TimeDelta::Max() - TimeDelta::FromSeconds(1) == TimeDelta::Max()
>
> This CL fully implements saturation behavior, such that TimeDelta::Max()
> and TimeDelta::Min() are treated as positive and negative infinity
> regardless, with all the expected mathematical consequences.
>
> The one exception is that adding/subtracting a saturated TimeDelta to a
> TimeClass is still undefined. This was left undefined because we are not
> yet making a statement on whether TimeClass should be considered to have
> clamping or saturating behavior for values outsied of range.
>
> See the linked bug, or also the discussion at
> https://groups.google.com/a/chromium.org/forum/#!topic/platform-architecture-dev/S4umX2sOvrk
>
> Bug: 869387
> Change-Id: I2b3f98c69e7fc5030a6e210d75409d667b73c5ac
> Reviewed-on: https://chromium-review.googlesource.com/1155264
> Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#579969}

TBR=gab@chromium.org,smcgruer@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 869387
Change-Id: I7251e77bd66dc95c9b1aae16fbfc89acd239f19b
Reviewed-on: https://chromium-review.googlesource.com/1175470
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583650}
[modify] https://crrev.com/b1aaab647bd8905a1a16bee1bbefc8e44929070f/base/time/time.h
[modify] https://crrev.com/b1aaab647bd8905a1a16bee1bbefc8e44929070f/base/time/time_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment