1244491 - percentages reported by about:performance are off by a factor of 10

Status: RESOLVED FIXED

(Toolkit :: Performance Monitoring, defect)

Description

[pipcet]

about:performance contains code like the following:

        cachedElements.eltCPU.textContent = `CPU usage: ${Math.ceil(delta.diff.jank.totalCPUTime/delta.diff.deltaT)}%.`;

which divides a microsecond value by a millisecond value, then reports that as a percentage. That's off by a factor of ten.

Testing with

      window.setInterval(function () {
          var now = Date.now();
          while (Date.now() - now < 250.0);
      }, 1000.0);

(which busy-waits for 250 ms once a second, and should thus consume 25% of the CPU) reports

CPU usage: 247%.

So it appears we never correct for this factor.

Comment 1

[pipcet]

Attachment: 0001-Bug-1244491-about-performance-percentages-off-by-a-f.patch

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Oops, I didn't notice this bug report. Thanks for the patch.

That's weird. I'm quite willing to believe that the code divides microseconds by milliseconds, but shouldn't it be off by a factor of 1000, rather than 10?

Comment 3

[pipcet]

No (In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #2)
> That's weird. I'm quite willing to believe that the code divides
> microseconds by milliseconds, but shouldn't it be off by a factor of 1000,
> rather than 10?

We want the result to be 100 if the two time intervals are the same, because 100% is easier to read than 1.00, I think. So divide microseconds by microseconds and multiply by 100 to get a percentage, or divide microseconds by milliseconds, then divide by 1000, then multiply by 100. The last two steps can be combined into a division by 10.

For a single process using all of one CPU, the micro-by-milli division will yield 1000, but we want 100 (percent), thus the factor of 10.

(Another question is whether we want to divide by the number of CPU cores. I think with worker threads we can see numbers up to 400% on a quad-core system, which might confuse users; however, by far the most common case is to have a single main thread do all the work, and displaying 25% for a single blocked thread is more confusing than displaying 400% for an all-CPU-cores workload, so the current code makes the most sense to me.)

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Ah, of course. For the moment, we have not activated performance monitoring for workers, so we are not going to see anything > 100%.

By the way, if you want someone to notice a patch, you should mark it for review.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8714039 [details] [diff] [review]
0001-Bug-1244491-about-performance-percentages-off-by-a-f.patch

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

Patch looks good to me. Just one change before I can accept it.

Can you change the first line of the commit message to 1/ Remove the word [Patch]; 2/ Add ", r=Yoric" at the end, to mark that I was the reviewer?

Comment 6

[pipcet]

Attachment: 0001-Bug-1244491-about-performance-percentages-off-by-a-f.patch

Fix commit message, add reviewer.

Comment 7

[pipcet]

Thanks!

Is this okay?

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8730664 [details] [diff] [review]
0001-Bug-1244491-about-performance-percentages-off-by-a-f.patch

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

Looks good to me. Let's see how it behaves in tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75a8a2a4b50d

Comment 9

[David Teller [:Yoric] - still alive but not very active]

And... let's land it!

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0b6f231921d4

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0b6f231921d4