Closed Bug 1039924 Opened 10 years ago Closed 10 years ago

Make arithmetic on TimeDuration not wrap when TimeDuration could be Forever (or INT64_MIN)

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35

People

(Reporter: birtles, Assigned: birtles)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 8 obsolete files)

part 1 - Separate platform-dependent parts of TimeDuration into TimeDurationPlatformUtils
8.89 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
part 2 - Convert between Forever and Infinity when converting TimeDurations
3.15 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
part 3 - Templatize TimeDuration so it can support different behaviors with regards to tick count arithmetic
18.05 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
part 4 - Fill out TimeDurationValueCalculator and use it
6.38 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
part 5 - Add SaturatingTimeDuration
9.14 KB, patch
froydnj
: feedback+
Details | Diff | Splinter Review
part 6 - Use SaturatingTimeDuration for timing calculations
8.62 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 7 - Add unary minus operator to BaseTimeDuration
1.46 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
part 5 - Add ForeverSafeTimeDuration
9.21 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Splitting this off from bug 1038032 so we can land something simple there before uplift and produce a more general and robust solution here.
In order to have different templated versions of TimeDuration we first split out the platform-specific code so that this code doesn't need to concern itself with templates (and because putting template code in .cpp files is messy).
Now that the implementation of a number of TimeDuration methods have been delegated to a separate TimeDurationPlatformUtils class, it is easier to add consistent handling to the parameters passed to and from those methods. This patch adds checks when converting between TimeDurations and doubles so that Forever (represented by a tick count of INT64_MAX) is paired with PositiveInfinity, and INT64_MIN is paired with NegativeInfinity.
This patch prepares the way for having a separate SaturatingTimeDuration class by factoring TimeDuration into a templated base class: BaseTimeDuration. BaseTimeDuration takes a templated parameter, ValueCalculator, which is a helper object that defines how various arithmetic operations are performed on its mValue member (an int64_t count of ticks). This patch does not actually define or use the ValueCalculator parameter yet but simply performs the renaming and templatization. With regards to the templatization, arithmetic operators are defined to take objects with the same ValueCalculator template parameter (so that we don't, for example, apply non-saturating arithmetic to a SaturatingTimeDuration). However, comparison operators are defined to also operate on objects with a different ValueCalculator template parameter since comparison should be independent of the type of arithmetic used. Likewise, the constructor and assignment operator are defined to operate on objects with a different ValueCalculator template parameter so that objects can be converted from TimeDuration to SaturatingTimeDuration and vice-versa. The constructor is marked as explicit, however, so that we don't silently convert a SaturatingTimeDuration to a TimeDuration and unwittingly apply non-saturating arithmetic to a SaturatingTimeDuration. TimeDuration is defined as a specialization of BaseTimeDuration that uses TimeDurationValueCalculator as its ValueCalculator type. TimeDurationValueCalculator is filled-in in a subsequent patch.
This patch builds on the templatization from the previous patch to move arithmetic on the mValue member of BaseTimeDuration to the ValueCalculator template parameter. We would like to add an assertion to operator/ that the divisor is not zero but currently such an assertion fails on B2G ICS Emulator. This will be tracked down in a follow-up bug.
This patch adds another implementation of BaseTimeDuration's ValueCalculator template parameter that is careful to preserve Forever/-Forever values when performing arithmetic. It also defines a typedef for a specialization of BaseTimeDuration that uses this new ValueCalculator definition.
This patch takes the SaturatingTimeDuration defined in the previous patch and uses it within the calculation of animation timing for parameters that are expected to be +/- Forever.
I had a go at implementing a new SaturatingTimeDuration type. Daniel, Nathan, what do you think? Do you think this approach will work? I'm somewhat inclined to make it check for overflow from addition/multiplication etc. too but I haven't done that yet.
Blocks: 1043156
Flags: needinfo?(dholbert)
Flags: needinfo?(nfroyd)
Attached patch Roll-up patch of parts 1~7 (obsolete) — — Splinter Review
Comment on attachment 8462373 [details] [diff] [review] part 4 - Fill out TimeDurationValueCalculator and use it Review of attachment 8462373 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/TimeStamp.h @@ +312,5 @@ > + static int64_t Multiply(int64_t aA, T aB) > + { > + MOZ_ASSERT(std::numeric_limits<T>::is_integer, > + "Using integer multiplication routine with non-integer type." > + " Further specialization required"); I think it'd be slightly more idiomatic to use: #include "mozilla/TypeTraits.h" ... MOZ_ASSERT(IsInteger<T>::value, ...) ISTR that we didn't like <limits>/numeric_limits, but I cannot remember why...
(In reply to Brian Birtles (:birtles) from comment #8) > I had a go at implementing a new SaturatingTimeDuration type. > > Daniel, Nathan, what do you think? Do you think this approach will work? > > I'm somewhat inclined to make it check for overflow from > addition/multiplication etc. too but I haven't done that yet. This all looks pretty good. It is a little weird to be implementing IEEE semantics (and referring to them as such) with integers, but I guess it's not much weirder than what we had before.
Flags: needinfo?(nfroyd)
Comment on attachment 8462374 [details] [diff] [review] part 5 - Add SaturatingTimeDuration >+class SaturatingTimeDurationValueCalculator >+{ >+public: >+ static int64_t >+ Add(int64_t aA, int64_t aB) >+ { >+ if ((aA == INT64_MAX && aB == INT64_MIN) || >+ (aA == INT64_MIN && aB == INT64_MAX)) { >+ return 0; >+ } I'm not sure this behavior (inf + -inf = 0) necessarily makes sense... Mathematically, "infinity - infinity" is undefined -- not necessarily equal to 0. With float values, it returns NaN. Of course, we have to return *something* -- but I wonder if just dropping this clause and taking the next "if" check would be more useful here? (Or at least, simpler & no-less-useful) I think that would be effectively making +infinity "stickier", i.e. we'd saturate "more eagerly" at Forever() than at -INT64_MAX -- which might be good, since we do expose Forever() but don't expose a "NegativeForever()". Either way, though, it's probably worth having a comment here to document the reason (if any) for the choice. Same goes for the other mathematically-undefined cases in these functions (for e.g. Minus(inf,inf) & Divide(inf,inf)) >+ template <typename T> >+ static int64_t >+ Multiply(int64_t aA, T aB) { >+ if (aA == INT64_MAX || aA == INT64_MIN || >+ aB == INT64_MAX || aB == INT64_MIN) { >+ return (aA >= 0) ^ (aB < 0) ? INT64_MIN : INT64_MAX; IMHO this is clearer if the comparisons are consistent -- so, maybe tweak that last line to: return (aA >= 0) ^ (aB >= 0) ? INT64_MAX : INT64_MIN; Also, this clause covers "infinity * 0" which (per above) is really undefined, and hence merits at least a comment documenting the behavior that you're choosing for that case, and why (even if it's just for simplicity). Also, I'm not sure it makes any sense to compare aB to INT64_[MAX|MIN] here, given that aB's has "type T", i.e. it's template-defined. If T is a float type, we really should be comparing it against float-values for +/- infinity, right? (I'm not even sure whether INT64_MAX/MIN are exactly-representable as float / double values -- I suspect they aren't.) (Ah, you have a template-specialization for Multiply below, for double. Maybe we should do this INT64_MIN/MAX-considering stuff in a template-specialization where T=int64_t, and just have the generic templated version be a one-liner that returns aA * aB... or something like that? Or at least add a comment here noting that we *know* T is int64_t, if we do in fact know that...) >+ static double >+ DivideDouble(int64_t aA, int64_t aB) >+ { >+ if (aA == aB) { >+ return 1.0; >+ } Maybe worth asserting if aB is 0. (to catch divide-by-0) Callers should check for that, but if they don't, we can help with a MOZ_ASSERT. >+ static int64_t >+ Modulo(int64_t aA, int64_t aB) >+ { >+ if (aA == INT64_MAX || aA == INT64_MIN) { >+ return 0.0; >+ } nit: s/0.0/0/ (since the return type is int64_t, not double) Also: I don't immediately see why this behavior (infinity % N => 0) makes particular sense & is worth special-casing... (I'd think infinity % N is undefined, since it implicitly depends on dividing infinity by something.) If we need this particular behavior for some reason, it definitely merits an explanatory comment. >+ if (aB == INT64_MAX || aB == INT64_MIN) { >+ return aA; >+ } >+ >+ return aA % aB; Do we really need that last if-check? Won't the final aA % aB "just work" here? (And if not, is "return aA" really the right thing?) e.g. if aA and aB were both INT64_MAX, then aA % aB would produce 0, instead of returning aA like you have it doing. But that doesn't seem bad. I suppose if aA is INT64_MIN and aB is INT64_MAX, then we'd get -1, which is maybe-wrong, but I'm not sure that's a common enough operation (or an operation whose behavior we care enough about) to merit a special case... (And even if it merits a special case, I'm not sure why INT64_MIN is the "right answer" there.)
Comment on attachment 8462377 [details] [diff] [review] part 7 - Add unary minus operator to BaseTimeDuration One other nit, from skimming the patches: >Bug 1039924 part 7 - Add unary minus operator to BaseTimeDuration > >diff --git a/xpcom/ds/TimeStamp.h b/xpcom/ds/TimeStamp.h [...] >+ BaseTimeDuration<ValueCalculator> >+ operator-() >+ { >+ // We don't just use FromTicks(ValueCalculator::Subtract(0, mValue)) here >+ // since we'd like to be able to do -TimeDuration::Forever(). >+ int64_t ticks = mValue == INT64_MAX >+ ? INT64_MIN >+ : mValue == INT64_MIN ? INT64_MAX : -mValue; >+ return FromTicks(ticks); Firstly, do we actually depend on this? (i.e. do we negate TimeDuration variables whose values are arbitrary?) If this is really just so we can "do -TimeDuration::Forever()" (per the code comment), it seems like it might be better to expose a dedicated NegativeForever() method. But if we have "TimeDuration foo; [foo gets munged and ends up at Forever()]; SomeFunction(-foo);", then I agree we need something like this. Secondly: IMHO this compound ternary operator (to set 'ticks') might be a bit too nuts. :) I'd prefer the more traditional if/else cascade, I think -- more human-readable, easier to step through in a debugger, & equivalent binary-code-wise. (albeit a few lines longer) Anyway -- at a high level, I think the approach of the patches here seems reasonable.
Flags: needinfo?(dholbert)
Thanks Daniel for the feedback! That all makes sense and I agree with the comments. I'm going to be away for the next week and a bit (and quite busy when I get back) and I don't think there's any urgency here so please excuse me if it takes me a while to get back to this. Given your detailed suggestions above, once I've addressed those issues I might just ask Nathan for review unless you'd like to have another look.
Blocks: 1066388
Attachment #8462370 - Attachment is obsolete: true
Attachment #8462371 - Attachment is obsolete: true
Attachment #8462372 - Attachment is obsolete: true
Attachment #8462373 - Attachment is obsolete: true
Attachment #8488386 - Flags: review?(nfroyd)
Attachment #8462374 - Attachment is obsolete: true
Attachment #8462376 - Attachment is obsolete: true
Attachment #8462377 - Attachment is obsolete: true
Attachment #8462380 - Attachment is obsolete: true
Attachment #8488382 - Flags: review?(nfroyd) → review+
Comment on attachment 8488383 [details] [diff] [review] part 2 - Convert between Forever and Infinity when converting TimeDurations Review of attachment 8488383 [details] [diff] [review]: ----------------------------------------------------------------- I am a little nervous about explicitly introducing infinities into the general-purpose API here, but I'm willing to see how this works.
Attachment #8488383 - Flags: review?(nfroyd) → review+
Comment on attachment 8488384 [details] [diff] [review] part 3 - Templatize TimeDuration so it can support different behaviors with regards to tick count arithmetic Review of attachment 8488384 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/TimeStamp.h @@ +70,5 @@ > { > MOZ_ASSERT(!aZero, "Who's playing funny games here?"); > } > + > + // Converting copy-constructor and assignment operators Could you please keep the comment about the default copy-constructor and assignment operators being OK? (Note that your template definitions do not stand in for the copy constructor and the assignment operator for the class itself.) @@ +77,5 @@ > + : mValue(aOther.mValue) > + { } > + > + template <typename E> > + BaseTimeDuration<ValueCalculator>& Here and throughout, you can just use |BaseTimeDuration| instead of |BaseTimeDuration<ValueCalculator>|. Makes things more readable.
Attachment #8488384 - Flags: review?(nfroyd) → review+
Comment on attachment 8488384 [details] [diff] [review] part 3 - Templatize TimeDuration so it can support different behaviors with regards to tick count arithmetic Review of attachment 8488384 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/TimeStamp.h @@ +266,5 @@ > // platforms with high-resolution timers. > > private: > friend class TimeStamp; > + friend struct IPC::ParamTraits<mozilla::BaseTimeDuration<ValueCalculator> >; We can use >> to close off these sorts of things now, no need for the empty space.
Comment on attachment 8488385 [details] [diff] [review] part 4 - Fill out TimeDurationValueCalculator and use it Review of attachment 8488385 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/TimeStamp.h @@ +316,5 @@ > > + template <typename T> > + static int64_t Multiply(int64_t aA, T aB) > + { > + MOZ_ASSERT(IsIntegral<T>::value, Please make this a static_assert; the static-ness of it should be delayed until template instantiation time.
Attachment #8488385 - Flags: review?(nfroyd) → review+
Comment on attachment 8488387 [details] [diff] [review] part 6 - Use SaturatingTimeDuration for timing calculations Review of attachment 8488387 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to defer to Daniel for this bit of the patch series. ::: xpcom/ds/SaturatingTimeDuration.h @@ +118,5 @@ > // -Forever / -x = Forever > if (aA == INT64_MAX || aA == INT64_MIN) { > return (aA >= 0) ^ (aB >= 0) > + ? NegativeInfinity<double>() > + : PositiveInfinity<double>(); This hunk goes in patch 5.
Attachment #8488387 - Flags: review?(nfroyd) → review?(dholbert)
Comment on attachment 8488388 [details] [diff] [review] part 7 - Add unary minus operator to BaseTimeDuration Review of attachment 8488388 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/TimeStamp.h @@ +165,5 @@ > { > mValue = ValueCalculator::Subtract(mValue, aOther.mValue); > return *this; > } > + BaseTimeDuration<ValueCalculator> |BaseTimeDuration| here, please.
Attachment #8488388 - Flags: review?(nfroyd) → review+
Comment on attachment 8488386 [details] [diff] [review] part 5 - Add SaturatingTimeDuration Review of attachment 8488386 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/SaturatingTimeDuration.h @@ +22,5 @@ > + * aA + aB > INT64_MAX (or < INT64_MIN). > + * > + * We currently don't check for that case since we don't expect that to > + * happen often except under test conditions in which case the wrapping > + * behavior is probably acceptable. I didn't really register this the first time through: this is sort of a weird way to do saturating arithmetic. So, really, the only ways to get to Forever are: 1. Exact construction; or 2. Lucky arithmetic. Is that right? WDYT about doing "real" saturated arithmetic here? I think mozilla::CheckedInt could probably help deal with most of the overflow detection issues. @@ +77,5 @@ > + template <typename T> > + static int64_t > + Multiply(int64_t aA, T aB) { > + // Specializations for double and int64_t are provided following. > + return aA * static_cast<int64_t>(aB); Shouldn't this really be: return Multiply(aA, static_cast<int64_t>(aB)); ? Otherwise if aA was INT64_{MIN,MAX}, you're violating the saturation contract. Also, do we care about T==float? In that case, we really want to take the double specialization so we get proper handling of infinities. @@ +118,5 @@ > + // -Forever / -x = Forever > + if (aA == INT64_MAX || aA == INT64_MIN) { > + return (aA >= 0) ^ (aB >= 0) > + ? NegativeInfinity<double> > + : PositiveInfinity<double>; These should be |NegativeInfinity<double>()| and |PositiveInfinity<double>()|; you fixed them up in patch 6, but the fixup really belongs here. @@ +181,5 @@ > + return (aA >= 0) ^ (aB >= 0.0) ? INT64_MAX : INT64_MIN; > + } > + > + double doubleResult = static_cast<double>(aA) * aB; > + if (doubleResult > INT64_MAX) { Ah, here you're implementing "proper" saturated arithmetic... :)
(In reply to Nathan Froyd (:froydnj) from comment #30) > I didn't really register this the first time through: this is sort of a > weird way to do saturating arithmetic. So, really, the only ways to get to > Forever are: > > 1. Exact construction; or > 2. Lucky arithmetic. > > Is that right? WDYT about doing "real" saturated arithmetic here? I think that's intentionally a non-goal here, at least for animation code, because: * It's too hard to get right in all of the cases (basically, every single arithmetic operation has to be treated as suspect), and the overhead isn't worth it. * It's unclear that it's even useful in any real-world scenarios. It *might* be helpful for e.g. animations that start at time INT64_MAX - 1 second, but nobody is going to actually create those in the real world. And even if they do, they're going to run into trouble regardless when they go past 1 second of animation, since we can't represent the time anymore other than just recognizing that we're stuck at ::Forever(). So, we're not really concerned with lucky [huge] arithmetic. Rather, the goal here is to allow ourselves to have easy special-case behavior for scenarios where we expect explicitly-infinite durations, e.g. to represent the runtime of an animation that never stops repeating (which authors can ask for).
(In reply to Daniel Holbert [:dholbert] from comment #31) > So, we're not really concerned with lucky [huge] arithmetic. Rather, the > goal here is to allow ourselves to have easy special-case behavior for > scenarios where we expect explicitly-infinite durations, e.g. to represent > the runtime of an animation that never stops repeating (which authors can > ask for). OK. I guess I am then confused why Multiply implements real saturating semantics while everything else quietly does something different. I would like to suggest a different color of paint for the bikeshed, but none of the colors I can come up with sound appropriate. FencepostedTimeDuration? BoundedTimeDuration? BlackHoleTimeDuration?
Comment on attachment 8488387 [details] [diff] [review] part 6 - Use SaturatingTimeDuration for timing calculations >-TimeDuration >+SaturatingTimeDuration > Animation::ActiveDuration(const AnimationTiming& aTiming) > if (aTiming.mIterationCount == mozilla::PositiveInfinity<float>()) { [...] > } >- return aTiming.mIterationDuration.MultDouble(aTiming.mIterationCount); >+ return SaturatingTimeDuration(aTiming.mIterationDuration) >+ .MultDouble(aTiming.mIterationCount); > } I think the SaturatingTimeDuration() conversion in the return statement here is a bit premature, and (as a result) a bit misleading. At least -- when I read it and thought about why it was there, I thought "Oh right, we have to convert before multiplying, because mIterationCount might be infinity. So we need saturating multiplication." But that's wrong, because this is in the *non*-infinite-mIterationCount case. [per the "if" check & early-return above it] So the "saturating" type doesn't impact the MultDouble call at all. *Really*, there's only one reason this SaturatingTimeDuration() conversion is there: so that we can conform to the function's return type. (I think?) So, we should do that conversion as the last possible step, as the value gets returned -- e.g.: return SaturatingTimeDuration( aTiming.mIterationDuration.MultDouble(aTiming.mIterationCount)); Does that sound OK? >diff --git a/dom/animation/Animation.h b/dom/animation/Animation.h > // The total duration of the animation including all iterations. > // Will equal TimeDuration::Forever() if the animation repeats indefinitely. >- TimeDuration mActiveDuration; >+ SaturatingTimeDuration mActiveDuration; Should this documentation now say "SaturatingTimeDuration::Forever()"? (maybe it compares as operator== to both of them?) >- static TimeDuration ActiveDuration(const AnimationTiming& aTiming); >+ static mozilla::SaturatingTimeDuration >+ ActiveDuration(const AnimationTiming& aTiming); We don't need the "mozilla::" prefix on SaturatingTimeDuration here, because this is inside a 'namespace mozilla {' block. (Note that we didn't have it on TimeDuration in the line that's being removed, and we don't have it on the AnimationTiming& aTiming parameter.) >diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp > TimeDuration elapsedTime = >- std::min(anim->InitialAdvance(), computedTiming.mActiveDuration); >+ std::min(anim->InitialAdvance(), >+ TimeDuration(computedTiming.mActiveDuration)); > AnimationEventInfo ei(aCollection->mElement, > player->Name(), NS_ANIMATION_START, > elapsedTime, aCollection->PseudoElement()); > aEventsToDispatch.AppendElement(ei); > } [...] > // Dispatch 'animationend' when needed. > AnimationEventInfo ei(aCollection->mElement, > player->Name(), NS_ANIMATION_END, >- computedTiming.mActiveDuration, >+ TimeDuration(computedTiming.mActiveDuration), > aCollection->PseudoElement()); > aEventsToDispatch.AppendElement(ei); > } I'm a little uneasy about the casual TimeDuration down-conversion here. (2 instances) It seems to me like once we've got a SaturatedTimeDuration, we should *never* convert it back to TimeDuration, except in rare circumstances where we're sure that no arithmetic is going to be done with it. (But even then, it seems like we could just as easily keep it Saturated.) I'd feel better if you did one of the following here: (1) Drop these down-conversions, & add an additional AnimationEventInfo constructor that takes a SaturatedTimeDuration (it'd look like the other constructor -- maybe they can share code). This lets us avoid the downconversion (or perhaps do it in an extremely-scoped place where it's clear that no arithmetic will happen with the result). ...OR: (2) Add a comment above these conversions, noting that we're intentionally dropping the "Saturating" behavior just so that we can match the AnimationEventInfo constructor-API, and that we know this is safe because AnimationEventInfo doesn't do any arithmetic with its TimeDuration. r=me with the above addressed.
Attachment #8488387 - Flags: review?(dholbert) → review+
(In reply to Nathan Froyd (:froydnj) from comment #32) > OK. I guess I am then confused why Multiply implements real saturating > semantics while everything else quietly does something different. Ah, good point. (This is for the "proper" saturating check at the end of comment 30.) I suspect that's intended to handle cases where an author explicitly specifies an extremely large iteration-count, and expects it to behave like "infinite". I'd imagine this "proper" check should help for situations like that, but not reliably, because if the author undershoots INT64_MAX (and, say, has an animation-delay), they could still run into trouble with subsequent arithmetic. My preference would be to remove this check, since it adds overhead and only provides a partial solution to an out-of-scope problem. Better to keep things simple and just have a "forever or not-forever" binary condition, IMHO. > I would like to suggest a different color of paint for the bikeshed, but > none of the colors I can come up with sound appropriate. > FencepostedTimeDuration? BoundedTimeDuration? BlackHoleTimeDuration? Yeah... I agree that "Saturating" might be misleading. The only alternate names I can think of sound a bit silly. (The best I can come up with is "ForeverSafeTimeDuration", with documentation saying that it's "forever-safe", i.e. it can safely do arithmetic with ::Forever(). But I'm not sure I like that better than "Saturating".)
Comment on attachment 8488386 [details] [diff] [review] part 5 - Add SaturatingTimeDuration Review of attachment 8488386 [details] [diff] [review]: ----------------------------------------------------------------- Just going to f+ this for now, but assuming a reasonable resolution to the discussion, I will r+ this. Interested to hear Brian's thoughts on the points raised.
Attachment #8488386 - Flags: review?(nfroyd) → feedback+
Thanks for all your feedback Nathan and Daniel! Before I go ahead and make all the suggested changes, I'd like to work out what the goal should be. I'm somewhat inclined to make this do proper saturating arithmetic. It's easier to understand and it will probably save us a bug or two in the future where someone sets some animation parameters that, after we do arithmetic on them, land us in the overflow range and produces odd behavior. I understand that will introduce overhead but it's on an opt-in basis so it would only be animation consumers that bear the cost and I don't expect it to be a significant cost even then (since painting normally far outweighs any time spent in timing calculations). The other drawback of making this do proper saturating arithmetic is that the guideline of "when to use SaturatingTimeDuration vs TimeDuration" is a little blurry. With the patches as they currently stand the guideline is: "Use SaturatingTimeDuration if the value could be +/-Forever and you're going to do arithmetic on it." If we made this thing do proper saturating arithmetic it would be as before plus: "... or if you're going to do any arithmetic on a TimeDuration that could lead to overflow". If we don't make this do proper saturating arithmetic then I guess BoundedTimeDuration might work? What do you think Daniel?
Flags: needinfo?(dholbert)
(In reply to Brian Birtles (:birtles) from comment #36) > Before I go ahead and make all the suggested changes, I'd like to work out > what the goal should be. I'm somewhat inclined to make this do proper > saturating arithmetic. I lean against it. The current code we've got here (just treating ::Forever() as special (with one exception per comment 34) is a reasonable, direct, & relatively-simple way of addressing the problems that triggered the original bug here. Implementing "true" saturating math here seems like it'd be scope-creep, particularly if we want to handle all possible edge cases, and with no clear benefit. If it turns out we actually discover a strong need for "real" saturating logic here, we can always add that in later; but for now, I don't see there being a gain, aside from being able to handle animations with huge durations & a gazzillion iterations a little more correctly. That doesn't seem compelling, when there's already a declarative keyword for "keep iterating forever" that authors should probably be using if they run up against that problem.) > It's easier to understand I suppose, but only a little bit easier. "TimeDuration with special logic to check for & handle math with Forever() values" isn't too hard to understand." (though it is hard to reduce to a type name) > and it will probably save > us a bug or two in the future where someone sets some animation parameters > that, after we do arithmetic on them, land us in the overflow range and > produces odd behavior. I'm not convinced we can predict if & where real-world code will actually run into this sort of problem. Also, in layout code, we've explicitly decided not to solve this sort of integer-overflow thing; I'm not clear on how this is different. (We do have NSCoordSaturatingAdd/Subtract, but that's primarily supposed to be for cases where one of the values may *be* nscoord_MAX, IIRC.) > The other drawback of making this do proper saturating arithmetic is that > the guideline of "when to use SaturatingTimeDuration vs TimeDuration" is a > little blurry. With the patches as they currently stand the guideline is: > "Use SaturatingTimeDuration if the value could be +/-Forever and you're > going to do arithmetic on it." If we made this thing do proper saturating > arithmetic it would be as before plus: "... or if you're going to do any > arithmetic on a TimeDuration that could lead to overflow". I very much agree with this paragraph. In a world with an overflow-safe TimeDuration, you'd really only be justified using a "vanilla" TimeDuration if you could *prove* that the operation you're doing won't overflow. And then, you might as well just trust the hypothetical TrulySaturatingTimeDuration class to prove it for you. So it seems like TimeDuration would just become obsolete, in that world. > If we don't make this do proper saturating arithmetic then I guess > BoundedTimeDuration might work? That sounds fine to me. We can always rename it later if we come up with a better name, I suppose. :)
Flags: needinfo?(dholbert)
(a few more thoughts on alternate names: "StickyTimeDuration" -- since Forever() is "sticky". FSTimeDuration -- with "FS" being an abbreviation for "forever-safe", from end of comment 34)
(In reply to Daniel Holbert [:dholbert] from comment #38) > (In reply to Brian Birtles (:birtles) from comment #36) > > Before I go ahead and make all the suggested changes, I'd like to work out > > what the goal should be. I'm somewhat inclined to make this do proper > > saturating arithmetic. > > I lean against it. > > The current code we've got here (just treating ::Forever() as special (with > one exception per comment 34) is a reasonable, direct, & relatively-simple > way of addressing the problems that triggered the original bug here. > > Implementing "true" saturating math here seems like it'd be scope-creep, > particularly if we want to handle all possible edge cases, and with no clear > benefit. Ok, you've persuaded me. I think ForeverSafeTimeDuration is ok?
Removed saturating handling from double version of Multiply
Attachment #8492818 - Flags: review?(nfroyd)
Comment on attachment 8492818 [details] [diff] [review] part 5 - Add ForeverSafeTimeDuration Review of attachment 8492818 [details] [diff] [review]: ----------------------------------------------------------------- I like Daniel's suggestion of 'StickyTimeDuration' better than 'ForeverSafeTimeDuration'. I think 'ForeverSafe' is too easily translated as 'Forever Safe' (as opposed to TimeDuration, which is only sometimes safe?), rather than the intended 'Forever-Safe'. 'Sticky' also gets closer to the earlier 'Saturated' without the connotations of 'Saturated'. Thanks for working through the feedback here! ::: xpcom/ds/ForeverSafeTimeDuration.h @@ +185,5 @@ > +template <> > +inline int64_t > +ForeverSafeTimeDurationValueCalculator::Multiply<float>(int64_t aA, float aB) > +{ > + MOZ_ASSERT(IsInfinite(aB) == IsInfinite(static_cast<double>(aB)), Where is this IsInfinite coming from (I guess the same question could be asked of the double specialization above)? I'm assuming this is from mozilla/FloatingPoint.h; if so, can you please #include that header specifically above so we're not cargo-culting it from someplace else? I think this assertion is always true as written, but there's no harm in leaving it in.
Attachment #8492818 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #43) > Comment on attachment 8492818 [details] [diff] [review] > part 5 - Add ForeverSafeTimeDuration ... > Where is this IsInfinite coming from (I guess the same question could be > asked of the double specialization above)? I'm assuming this is from > mozilla/FloatingPoint.h; if so, can you please #include that header > specifically above so we're not cargo-culting it from someplace else? Oops, I missed this comment. I'll push a follow-up for this.
(In reply to Brian Birtles (:birtles) from comment #45) > Oops, I missed this comment. I'll push a follow-up for this. https://hg.mozilla.org/integration/mozilla-inbound/rev/9cd2473593c0
Depends on: 1134538
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: