852305 - immediately free LifoAlloc memory when we finish parsing huge scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: patch

Large Emscripten scripts can allocate gigabytes of ParseNodes.  We should eagerly free this memory after parsing completes to free it up for future allocations that happen before a GC.

Comment 1

[Nicholas Nethercote [inactive]]

Comment on attachment 726358 [details] [diff] [review]
patch

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

::: js/src/ds/LifoAlloc.cpp
@@ +68,5 @@
> +void
> +LifoAlloc::freeAllIfHugeAllocation()
> +{
> +    if (markCount)
> +        return;

I have a preference for the |markCount != 0| form when zero-testing integer variables.

The function name doesn't make it clear that markCount must be zero.  Perhaps freeAllIfEmptyAndHuge()?

@@ +72,5 @@
> +        return;
> +
> +    size_t accum = 0;
> +    for (BumpChunk *it = first; it; it = it->next())
> +        accum += it->getBumpSpaceSize();

I'm about to land (in the next hour or so, if people stop breaking things) bug 850523, which adds LifoAlloc::curSize_.  Just compare that against HUGE_ALLOCATION.

::: js/src/frontend/Parser.cpp
@@ +393,5 @@
>  {
>      JSContext *cx = context;
>      cx->tempLifoAlloc().release(tempPoolMark);
>      cx->activeCompilations--;
> +    cx->tempLifoAlloc().freeAllIfHugeAllocation();

A comment here might be helpful.

I also wonder if, instead of freeing everything if the allocation is huge, it would be better to free everything above a moderate threshold... maybe 128 KiB.  Then freeAllIfHugeAllocation() would be renamed freeExcessChunks() or something like that.

Comment 2

[Luke Wagner [:luke]]

(In reply to Nicholas Nethercote [:njn] from comment #1)

Thanks for the review!

> I also wonder if, instead of freeing everything if the allocation is huge,
> it would be better to free everything above a moderate threshold... maybe
> 128 KiB.

We could, but I doubt it would help much in the grand scheme of things.  For now I'd like to keep this function relatively simple since it is rare and LifoAlloc bugs are annoying.

Comment 3

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d54c51456af5

Comment 4

[Ed Morley [:emorley]]

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