1517354 - about:memory report formatting is slow

Status: RESOLVED FIXED

Description

[Cameron McCormack (:heycam)]

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.)

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1517354 - Improve about:memory performance by not using toLocaleString

Comment 2

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c4106e777ab7bf0dcf7d7eff0590c9b4e8e106b

Comment 3

[Nicholas Nethercote [:njn]]

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?

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a52b66841df9
Improve about:memory performance by not using toLocaleString r=njn

Comment 5

[Oana Pop-Rus]

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

Comment 6

[Jason Orendorff [:jorendorff]]

Thanks for the needinfo on this. By coincidence, I recently filed a bug about this. Bug 1514851.

Comment 7

[Jeff Walden [:Waldo]]

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.

Comment 8

[Jeff Walden [:Waldo]]

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