Closed Bug 1736489 Opened 1 month ago Closed 1 month ago

Counters should be output for each sample *before* a change

Categories

(Core :: Gecko Profiler, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox95 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

Details

Attachments

(2 files)

Spawned from bug 1735992 comment 4:
In the front-end, the graph draws straight lines between each counter sample.
This is incorrect in cases where some samples were skipped because they were the same as before.

Instead, between two different samples at (for example) t1 and t9, there should be a horizontal line between t1 and t8, and then a straight line between t8 and t9, because the memory change only really happened between the last two sample times.

I think we should fix this in the back-end, because otherwise the time of the last-sample-before-change is lost. The front-end could theoretically guess it from stack sample times, but I think the back-end has the actually correct information with the more precise time.

WIP patch on the way, but it's looking promising:

Profile before: https://share.firefox.dev/30DZm4m
Notice the big diagonal lines.
E.g., in Web Content (1/3) between 3.2s and 4.1s, making it seem like the memory use gradually increased during that time.

Profile after: https://share.firefox.dev/3lSVzIr
Notice the sharper changes.
E.g., in WebExtensions around 1.7s. Hovering over the memory graph, there is a flat line between 0s and 1.65s, then (zooming in), two diagonal lines between 1.650s, 1.652s, 1.654s (I was sampling with a 2ms interval), and then it's flat after that.
Without the patch, we may have seen a long angled line between 0s and 1.65s, but now we can see that the memory change really happened a few milliseconds around 1.65s, so the graph is overall much more correct.

This patch adds generic checks in all JSON profiles, that "counters" look valid.
And in GeckoProfiler.Counters, there are new checks for manually-added counters.

WaitForSamplingState had to be moved up. It's more efficient and reliable than an arbitraty PR_Sleep to wait for samples to be taken.

The makes the graphs much more correct:
If there were multiple skipped samples, the front-end would graph a long diagonal line between the samples before and after the skipped ones, making it look like the value gradually changed during that time.
Instead, by forcing the output of values before a change, this shows a better graph, with a horizontal line while the value didn't change, and a more abrupt diagonal line between the last two sample times when the change actually happened.

Also always output the very last sample, to clarify the final value (in case it was the same as previous ones.)

Depends on D128840

Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b2b1a2d91dc
Add gtests for counters in JSON profiles - r=florian
https://hg.mozilla.org/integration/autoland/rev/b98e0857d9ab
Always output the last counter sample before each change - r=florian
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch
You need to log in before you can comment on or make changes to this bug.