920909 - Plugin profiling syncing fails because float serialization is lossy

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

Currently we save startTime using ostream which on mac uses a scientific notation which truncates the least significant digits preventing the profile from syncing properly. Further more when this is fixed js floats have trouble dealing with these startTime as well.

Comment 1

[(no longer active)]

Comment on attachment 810399 [details] [diff] [review]
patch

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

::: tools/profiler/JSCustomObjectBuilder.cpp
@@ +158,5 @@
>  template <>
> +struct SendToStreamImpl<double>
> +{
> +  static void run(std::ostream& stream, double p) {
> +    stream << std::fixed << p;

According to http://www.cplusplus.com/reference/ios/ios_base/precision/, you want to use the default notation, and pass in a precision denoting the total number of significant digits in the mantissa.  The output will not be zero-padded, which is what you want here.

::: tools/profiler/TableTicker.cpp
@@ +102,5 @@
>    b.DefineProperty(meta, "jank", mJankOnly);
>    b.DefineProperty(meta, "processType", XRE_GetProcessType());
>  
>    TimeDuration delta = TimeStamp::Now() - sStartTime;
> +  // Don't use float to represent time since epoque because

epoch

@@ +105,5 @@
>    TimeDuration delta = TimeStamp::Now() - sStartTime;
> +  // Don't use float to represent time since epoque because
> +  // the js doesn't have enough precision to handle this
> +  // correctly.
> +  b.DefineProperty(meta, "startTime", PR_Now()/1000 - delta.ToMilliseconds());

ToMilliseconds() returns a double, so here you're just losing the least three significant digits of what PR_Now() returns, which does not seem to be what you want.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch

Much better!

Comment 3

[(no longer active)]

Comment on attachment 810899 [details] [diff] [review]
patch

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

::: tools/profiler/JSCustomObjectBuilder.cpp
@@ +160,5 @@
> +{
> +  static void run(std::ostream& stream, double p) {
> +    // 13 for ms, 16 of microseconds, plus an extra 2
> +    stream.precision(18);
> +    stream << std::fixed << p;

You don't want std::fixed here like I said earlier.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 5

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc2c853d5c76

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cc2c853d5c76