1143508 - JSDOMParser memory usage optimizations

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Nicholas Nethercote [inactive]]

While profiling some other code I saw JSDOMParser come up. It's doing some unnecessary allocations that are easy to work around.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Optimize tag name scanning in JSDOMParser

On one workload I saw this reduce the cumulative allocations done for strings
drop from 2.3 MB to 0.9 MB.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Optimize pair returning from makeNodeElement() in JSDOMParser

In one workload this avoids allocating 2.3 MB of short-lived arrays.

Comment 3

[Nicholas Nethercote [inactive]]

There are some slightly more invasive things that could be done -- e.g. lazily creating the |attributes| and |childNodes| members of |Element| -- but I don't have a good sense of how big are the workloads that this code processes. If they are large, then these more invasive things might be worthwhile.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> There are some slightly more invasive things that could be done -- e.g.
> lazily creating the |attributes| and |childNodes| members of |Element| --
> but I don't have a good sense of how big are the workloads that this code
> processes. If they are large, then these more invasive things might be
> worthwhile.

We can get some typical raw articles and profile them to see how |attributes| and |childNodes| are used. Looking at Readability.js [1] I see |attributes| used in a cleaner function, used to strip unwanted tags. I see |childNodes| used in more places.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/Readability.js

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8577793 [details] [diff] [review]
(part 1) - Optimize tag name scanning in JSDOMParser

Do we get a benefit of keeping the array as a class member, or can we use a local version?

Comment 6

[Nicholas Nethercote [inactive]]

> Do we get a benefit of keeping the array as a class member, or can we use a
> local version?

The last sentence of the big comment addresses this -- with a local version a new array will get allocated every time. More specifically, this function is hot and the length of the string is typically short (e.g. 1--6 chars) so the difference is significant.

Comment 7

[:Margaret Leibovic]

This will also help with the desktop memory issues we've been seeing (e.g. bug 1142183).

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8577795 [details] [diff] [review]
(part 2) - Optimize pair returning from makeNodeElement() in JSDOMParser

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

Thanks!

Comment 9

[Nicholas Nethercote [inactive]]

Thank you for the reviews.

https://hg.mozilla.org/integration/mozilla-inbound/rev/277d225086df
https://hg.mozilla.org/integration/mozilla-inbound/rev/7480a3b3c5d1

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/277d225086df
https://hg.mozilla.org/mozilla-central/rev/7480a3b3c5d1

Comment 11

[Nicholas Nethercote [inactive]]

Apparently this improved Talos:

Improvement: Mozilla-Inbound-Non-PGO - Tp5 Optimized (Private Bytes) - WINNT 6.1 (ix) - 5.28% decrease
------------------------------------------------------------------------------------------------------
    Previous: avg 308942833.333 stddev 5020927.890 of 12 runs up to revision 24640e5e0d3d
    New     : avg 292633166.667 stddev 3765593.756 of 12 runs since revision 7480a3b3c5d1
    Change  : -16309666.667 (5.28% / z=3.248)
    Graph   : http://mzl.la/1Cr3BgB

I'm very surprised. It was only a small optimization, and Reader View shouldn't be contributing that much to memory usage that avoiding some short-lived GC things should result in a 5% improvement in total memory usage.

Comment 12

[:Margaret Leibovic]

(In reply to Nicholas Nethercote [:njn] from comment #11)
> Apparently this improved Talos:
> 
> Improvement: Mozilla-Inbound-Non-PGO - Tp5 Optimized (Private Bytes) - WINNT
> 6.1 (ix) - 5.28% decrease
> -----------------------------------------------------------------------------
> -------------------------
>     Previous: avg 308942833.333 stddev 5020927.890 of 12 runs up to revision
> 24640e5e0d3d
>     New     : avg 292633166.667 stddev 3765593.756 of 12 runs since revision
> 7480a3b3c5d1
>     Change  : -16309666.667 (5.28% / z=3.248)
>     Graph   : http://mzl.la/1Cr3BgB
> 
> I'm very surprised. It was only a small optimization, and Reader View
> shouldn't be contributing that much to memory usage that avoiding some
> short-lived GC things should result in a 5% improvement in total memory
> usage.

There were a bunch of talos regressions caused by enabling background parsing of every page (bug 1139678). I'm trying to land a patch to avoid doing that on desktop, but we're planning to keep that logic on Android for the near future.

If you think this patch is low-risk, I think we should uplift it to Fx38, since that's where we're planning to ship reader mode on desktop.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3af32770b0f3
https://hg.mozilla.org/releases/mozilla-aurora/rev/f2e7171388b1