853398 - |TimeDuration operator* (double aMultiplier)| should not floor aMultiplier

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Avi Halachmi (:avih)]

Attachment: TimeDuration mult: convert to integral after mult

With current behavior, aMultiplier is converted to an integral type before the multiplication, which makes this operator useless with non-integral values 0..1, and generally less accurate than it could be. We should convert to integral after the multiplication.

This seems a trivial fix, but maybe the elders know a good reason for the current behavior.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 727618 [details] [diff] [review]
TimeDuration mult: convert to integral after mult

Review of attachment 727618 [details] [diff] [review]:
-----------------------------------------------------------------

This will loose the bottom bits of duration when it is converted to a double. On OS X durations are stored in nanoseconds which means we'll start loosing precision on
durations of about 52 days.

This will also be slower than previous code because it does a conversion to double, a double multiplication and then a conversion back from double. The previous code
could easily avoid any doubles if the the multiplier was known at compile time.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So the patch that added this operator only passes "2.0" as the double value, as far as I can tell.

So why do we have this version of the operator anyway?

Avi, I assume you were planning to use it somewhere, which is how you ran into this problem?

Comment 3

[Avi Halachmi (:avih)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> So why do we have this version of the operator anyway?

Good question. If it would have accepted integrals only, at least we would get a warning (or possibly an error even), but as is, it's plain deception to me.

> Avi, I assume you were planning to use it somewhere, which is how you ran
> into this problem?

Indeed. Enable vsync timing: https://tbpl.mozilla.org/?tree=Try&rev=8e03d19dc637 (this includes this patch, and I don't see obvious broken stuff from it at the try tests).

It's not hard to work around this (e.g. TimeDuration half = TimeDuration::FromMilliseconds(someDuration.ToMilliseconds() * 0.5); ). But I wasted too much time on this, as it was the last thing I suspected to be broken. So to save the time of others, I suggest to either remove the overloaded * (double) or just make it work better.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I think we should remove the implementation of this method and mark it as MOZ_DELETE to catch callers passing floats...

Comment 5

[Avi Halachmi (:avih)]

I'll solve this, so just an interesting update for now:

replacing the body of operator* with MOZ_DELETE fails with a somewhat weird message on an existing line of code. There is an expression of |aTimeDuration * (someProperIntValue + 1)| which now produces a message about ambiguity. The 3 other overloads of operator* accept int32_t, int64_t and uint32_t. Supposedly it should have fitted the int32_t one perfectly, and yet...

https://tbpl.mozilla.org/?tree=Try&rev=ea2ca85312be

The fedora debug error message:
RefreshDriver.cpp: In member function 'virtual void mozilla::PreciseRefreshDriverTimer::ScheduleNextTick(mozilla::TimeStamp)':
../../../layout/base/nsRefreshDriver.cpp:269:81: error: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second:
../../dist/include/mozilla/TimeStamp.h:100:16: note: candidate 1: mozilla::TimeDuration mozilla::TimeDuration::operator*(int32_t) const
../../dist/include/mozilla/TimeStamp.h:98:16: note: candidate 2: mozilla::TimeDuration mozilla::TimeDuration::operator*(double) <deleted>

The windows error message:
e:/dev/moz/src/central/layout/base/nsRefreshDriver.cpp(269) : error C2666: 'mozilla::TimeDuration::operator *' : 4 overloads have similar conversions

Comment 6

[Avi Halachmi (:avih)]

Attachment: V3 - Disable operator* using MOZ_DELETE, provide explicit MultDouble

Previous push didn't have const, didn't set the method to private as suggested at MOZ_DELETE docs. I also added an explicit alternative: MultDouble.

Try build with MOZ_DELETE:
https://tbpl.mozilla.org/?tree=Try&rev=96e9e9abba93

Try build to verify that operator* for double and float fail on all platforms:
https://tbpl.mozilla.org/?tree=Try&rev=193464a18381

Comment 7

[Avi Halachmi (:avih)]

Comment on attachment 728083 [details] [diff] [review]
V3 - Disable operator* using MOZ_DELETE, provide explicit MultDouble

Looks good.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 728083 [details] [diff] [review]
V3 - Disable operator* using MOZ_DELETE, provide explicit MultDouble

>+  // If required, use MultDuoble explicitly and with care.

Please fix the spelling.

r=me

Comment 9

[Avi Halachmi (:avih)]

Attachment: v4 - Address comment #8

Comment 10

[Vladan Djeric (:vladan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90e1cafdd9c6

Comment 11

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/90e1cafdd9c6