Closed Bug 1149897 Opened 6 years ago Closed 6 years ago

PerformanceStatsService data is not monotonic


(Toolkit :: Performance Monitoring, defect)

Not set



Tracking Status
firefox40 --- fixed


(Reporter: Yoric, Assigned: Yoric)




(1 file, 1 obsolete file)

After switching from 16ms resolution to 1ms resolution, it appears that the data provided by the PerformanceStatsService is not monotonic. We should work around that limitation.
Attached patch monotonic.diff (obsolete) — Splinter Review
This patch implements a lightweight fix to force the times used by the stopwatch monitoring to be monotonic. Also, it reactivates a subtest deactivated in bug 674779, minus a subsubtest whose fix will be more sophisticated and will need bug 1150045.

Incidentally, I suspect that the failures in the later subsubtest is due to a previous test using Jetpack to build an add-on but not cleaning up properly after itself (reading list? loop?)
Assignee: nobody → dteller
Attachment #8586822 - Flags: review?(jdemooij)
Component: General → Performance Monitoring
Comment on attachment 8586822 [details] [diff] [review]

Review of attachment 8586822 [details] [diff] [review]:

::: js/src/vm/Interpreter.cpp
@@ +567,5 @@
>          kernelTimeInt.LowPart = kernelFileTime.dwLowDateTime;
>          kernelTimeInt.HighPart = kernelFileTime.dwHighDateTime;
> +        // Convert 100 ns to 1 us, make sure that the result is monotonic
> +        *systemTime = runtime_->
> +            stopwatch.systemTimeFix.monotonize(kernelTimeInt.QuadPart / 10);

This fits on one line (99 chars)

@@ +573,5 @@
>          userTimeInt.LowPart = userFileTime.dwLowDateTime;
>          userTimeInt.HighPart = userFileTime.dwHighDateTime;
> +        // Convert 100 ns to 1 us, make sure that the result is monotonic
> +        *userTime = runtime_->
> +            stopwatch.userTimeFix.monotonize(userTimeInt.QuadPart / 10);

And here.
Attachment #8586822 - Flags: review?(jdemooij) → review+
Applied feedback.
Attachment #8586822 - Attachment is obsolete: true
Attachment #8587446 - Flags: review+
I believe that what you encountered is actually bug 1150259, which is anterior. I'll probably want to deactivate the test under Windows XP.
Flags: needinfo?(dteller)
So, Wes, how do you want to handle this? I have just put up for review a wallpaper patch for bug 1150259, btw.
Flags: needinfo?(wkocher)
Disabling your way to victory is more than acceptable to me. :)
Flags: needinfo?(wkocher)
In that case, let's land this bug, and let's handle bug 1150259 in bug 1150259.
Keywords: checkin-needed
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.