about:memory report formatting is slow

RESOLVED FIXED in Firefox 66

Status

()

enhancement
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

unspecified
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

6 months ago
Loading a large memory report in about:memory is slow.  For example when I load a memory report file with around 12,000 reports in it, the main thread is blocked for around 2 s.

Profiling reveals that more than half of the time is spent in Number.toLocaleString.  That's unfortunate, since it's a convenient API, but reimplementing just what we need helps a lot.  With the forthcoming patch, the time spent under formatNum on this file I was testing with drops from 1200 ms to 10 ms.

(Making report formatting faster is important for bug 1517175, where my patch will reformat upon typing in a new filter string.)
Aw geez, I just got rid of the old hand-rolled formatting code in bug 1499906 :(

The fact that the JS code in the patch, which doesn't look particularly fast, is nonetheless vastly faster than toLocaleString() suggests that the implementation of toLocaleString() has perf problems.

Jason, do you have any thoughts about that?
Flags: needinfo?(jorendorff)

Comment 4

5 months ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a52b66841df9
Improve about:memory performance by not using toLocaleString r=njn

Comment 5

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a52b66841df9
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Thanks for the needinfo on this. By coincidence, I recently filed a bug about this. Bug 1514851.
Flags: needinfo?(jorendorff)

toLocaleString will recompute/recreate an ICU formatting object every time you call it. The intended way users are expected to adapt to that, is to create a formatting object once, then use that repeatedly. We could perhaps add caching or something for commonly-specified sets of options, or a one-element LRU cache, or similar, but that would slow down every user for the few that don't realize they should be creating a formatter object.

But it looks near-trivial to make this use formatter objects instead of toLocaleString, so we should just do that. I'll file a bug to do that, with an untested patch I've just churned out locally that hopefully someone else can push over the line.

Filed bug 1518263 to use formatter objects for this. The way formatNum is used here is exactly amenable to using cached formatter objects.

You need to log in before you can comment on or make changes to this bug.