Use saturation computation in base::TimeDelta |
|
Issue descriptionCurrently, TimeDelta has saturation behavior for translating from/to other values, but clamping behavior for computation. For example: TimeDelta::Max() + TimeDelta::FromMicroseconds(100) == TimeDelta::Max() TimeDelta::Max() - TimeDelta::FromMicroseconds(100) != TimeDelta::Max() This mismatch causes strange (albeit rare) behavior, in that: TimeDelta::Max().InSecondsF() --> infinity TimeDelta::Max().InSecondsF() - 100 --> infinity (TimeDelta::Max() - TimeDelta::FromMicroseconds(100)).InSecondsF() --> finite-value It also means TimeDelta is not a fit for blink Animations, where there are time values that can be infinite by spec and should follow proper mathematics on them. After some discussion in https://groups.google.com/a/chromium.org/forum/#!topic/platform-architecture-dev/S4umX2sOvrk it was agreed that we should give TimeDelta saturation mathematics if possible (e.g. if there are no performance or existing-code concerns). See also this document which summarizes the current situation: https://docs.google.com/document/d/1cwmebJmKrKHCeuZUNARuRo6lIKQDicZkqfQrLHWsLnE/view#
,
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
,
Aug 17
Note: this was confirmed as the cause of the perf regression in issue 872123 , so that will need tackled as we look to a rework of the CL. |
|
►
Sign in to add a comment |
|
Comment 1 by bugdroid1@chromium.org
, Aug 1