Closed Bug 1143508 Opened 5 years ago Closed 5 years ago

JSDOMParser memory usage optimizations

Categories

(Toolkit :: Reader Mode, defect)

Other
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(2 files)

While profiling some other code I saw JSDOMParser come up. It's doing some unnecessary allocations that are easy to work around.
On one workload I saw this reduce the cumulative allocations done for strings
drop from 2.3 MB to 0.9 MB.
Attachment #8577793 - Flags: review?(bnicholson)
In one workload this avoids allocating 2.3 MB of short-lived arrays.
Attachment #8577795 - Flags: review?(bnicholson)
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.
(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 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?
> 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.
This will also help with the desktop memory issues we've been seeing (e.g. bug 1142183).
Component: Reader View → Reader Mode
Product: Firefox for Android → Toolkit
Version: Firefox 32 → Trunk
Attachment #8577793 - Flags: review?(bnicholson) → review+
Comment on attachment 8577795 [details] [diff] [review]
(part 2) - Optimize pair returning from makeNodeElement() in JSDOMParser

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

Thanks!
Attachment #8577795 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/277d225086df
https://hg.mozilla.org/mozilla-central/rev/7480a3b3c5d1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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.
(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.
You need to log in before you can comment on or make changes to this bug.