892097 - LifoAlloc: Reduce freeAllIfHuge bound on B2G.

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

This bug is created to handle the patch which was supposed to improve the memory consumption on B2G after the parser has finished running (see Bug 876906).

This is a concern because the memory used by the parser might cause some OOM which can be avoided if we set a lower bound for Huge (currently 50 MB).

The idea would be to set a global preference to set this limit in the pref.js files instead of having a #ifdef as the patch in Bug 876906 did.  The reason why it was done that way in Bug 876906 was to be able to backport it to b2g18 easily, but this is no longer a concern as the patch will only delay the OOM to a later point in the case of Pdf.js

Comment 1

[Nicholas Nethercote [inactive]]

I *think* that lazy parsing (bug 678037, or maybe it was bug 835587) has greatly reduced the peak memory consumption during parsing in FF24.  I don't entirely understand the situation here, but if you're talking about a patch that will land on trunk (as opposed to b2g18) you should re-measure pdf.js to see if it is still needed.  I measured parsenode usage in Octane's copy of pdf.js the other day in Firefox and it peaked at only 1.7 MiB.

Comment 2

[Nicolas B. Pierron [:nbp]]

Indeed, I do not think this would be as helpful as it would have been on b2g18, but I guess this might still be useful for the first reason this code had been added, which is for asm.js code.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Add preference for changing lifo alloc freeAllIfHuge threshold.

This is the sane non-backportable version as attachement 769219 provided in Bug 876906.

This patch add a preference as part of the JSRuntime to lower the threshold in the preference file of b2g.

Comment 4

[Luke Wagner [:luke]]

Are you quite sure we need this pref?  I was thinking that it might instead make sense to internalize the notion of a memory-constrained environment by having a --optimize-for-space configure option that we could use to coherently control this and the multitude of other arbitrary limits that affect memory usage.  Thoughts on this?

Comment 5

[Terrence Cole [:terrence]]

I've been thinking along the same lines. When I suggested it to Bill, he seemed to prefer continuing to use individual tunable parameters.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Luke Wagner [:luke] from comment #4)
> Are you quite sure we need this pref?  I was thinking that it might instead
> make sense to internalize the notion of a memory-constrained environment by
> having a --optimize-for-space configure option that we could use to
> coherently control this and the multitude of other arbitrary limits that
> affect memory usage.  Thoughts on this?

I think we have 2 issues, there is a locality issue and a variability feature.

Indeed, I do not think it is as important to have the variability part on this threshold as this is not a primary performance-wise setting (that we want to check without recompiling).

However, I think we should isolate this settings to be closer to other settings such as we do not have to search across all files to find such parameters.

I think it makes sense to have an --optimize-for-space configure options if we want to provide a different algorithm without having the code-size of the other algorithm, or the corner cases of the other algorithm.

Wrapping constants in such #ifdef around the code sounds like something would be forgotten.

If we want to go to the #ifdef wrapping constants, we should at least make sure that all the variables are in a well defined place.  This is also one thing I am interested in doing as part of the heuristics refactoring, such as we can limit the number of locations.

In the mean time, having an isolated limit is bad and moving it to the JSRuntime sounds like something which should be done.

Comment 7

[Luke Wagner [:luke]]

I definitely am not arguing against *all* prefs, just ones where it seems like we wouldn't get that much mileage from quick experimentation etc.  To avoid making mobile behave much differently than fat desktops (leading to, e.g., mobile-only regressions), perhaps we should just tweak this particular parameter to the lower one suitable for mobile.

Comment 8

[Nicholas Nethercote [inactive]]

This bug feels like it's going to go nowhere.  For the LifoAlloc issue, the combination of lazy parsing and the asm.js-specific parsing changes have completely fixed the parsing memory spikes, AIUI.  So if this bug is about LifoAlloc I think it can be closed.

If this bug is about a more general --optimize-for-space flag, that's a reasonable idea, but the bug should be retitled accordingly.  nbp?

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> This bug feels like it's going to go nowhere.  For the LifoAlloc issue, the
> combination of lazy parsing and the asm.js-specific parsing changes have
> completely fixed the parsing memory spikes, AIUI.  So if this bug is about
> LifoAlloc I think it can be closed.
> 
> If this bug is about a more general --optimize-for-space flag, that's a
> reasonable idea, but the bug should be retitled accordingly.  nbp?

This bug was about the memory spike at parse time. (bug 876906), I think the patch would be interesting if we add other abusive use of LifoAlloc.  It does not make sense to make it for optimize for space, as freeAllIfHuge is only used in the Parser.

I will mark it as WORKS-FOR-ME, as the problem no longer exists on new versions of Gecko, and as commented in Bug 876906, this does not solve the original issue.