1084177 - Reduce heap churn caused by JS source compression

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [:njn] (On Long Service Leave)]

I did some heap profiling with DMD configured to ignore all calls to free() and |delete|. This means it profiles all heap allocations, rather than just the live ones. Among other things, this identifies areas of high heap churn.

By far the biggest cause of churn was allocations with stack trace, and four others like it:

>  #01: MOZ_Z_deflateInit2_ (modules/zlib/src/deflate.c:294)
>  #02: MOZ_Z_deflateInit_ (modules/zlib/src/deflate.c:207)
>  #03: js::Compressor::init() (js/src/vm/Compression.cpp:62)
>  #04: js::SourceCompressionTask::work() (js/src/jsscript.cpp:1736)
>  #05: js::HelperThread::handleCompressionWorkload() (js/src/vm/HelperThreads.cpp:1188)

There are five ZALLOC() calls in deflateInit2_. In this context they allocate 5936, 65536, 65536, 65536 and 65536 bytes respectively, for a total of 268080 bytes. And we create one of these structs for every script that's compressed.

The structs are short-lived, but still... just starting Firefox on Linux64 I saw 427 of these structs created, which is 114 MB, which is 26% of the cumulative heap allocations. After then loading Gmail the number jumped to 909 (244 MB).

Could we create just one struct and re-use it? Or perhaps one per helper thread?

Comment 1

[:Benjamin Peterson]

Attachment: One z_stream per helper thread POC

It's fairly easy to create only one compression context per helper thread. Here's a POC patch. njn, can you see if this patch improves the situation and is worth tidying up?

More broadly, though zlib has gotten us far, we should perhaps look at snappy. It might have a lighter footprint. I see also that asmjs is using LZ4.

Comment 2

[Nicholas Nethercote [:njn] (On Long Service Leave)]

The patch helps a lot! I did two runs with similar workloads. Without the
patch:

> Live or dead {
>   564 blocks in heap block record 4 of 1,524
>   36,962,304 bytes (36,962,304 requested / 0 slop)
>   Individual block sizes: 65,536 x 564
>   3.33% of the heap (19.96% cumulative)
>   Allocated at {
>     #01: MOZ_Z_deflateInit2_ (/home/njn/moz/mi4/co64dmd/modules/zlib/src/../../../../modules/zlib/src/deflate.c:294)
>   }
> }
> 
> Live or dead {
>   564 blocks in heap block record 5 of 1,524
>   36,962,304 bytes (36,962,304 requested / 0 slop)
>   Individual block sizes: 65,536 x 564
>   3.33% of the heap (23.29% cumulative)
>   Allocated at {
>     #01: MOZ_Z_deflateInit2_ (/home/njn/moz/mi4/co64dmd/modules/zlib/src/../../../../modules/zlib/src/deflate.c:299)
>   }
> }
> 
> Live or dead {
>   564 blocks in heap block record 6 of 1,524
>   36,962,304 bytes (36,962,304 requested / 0 slop)
>   Individual block sizes: 65,536 x 564
>   3.33% of the heap (26.62% cumulative)
>   Allocated at {
>     #01: MOZ_Z_deflateInit2_ (/home/njn/moz/mi4/co64dmd/modules/zlib/src/../../../../modules/zlib/src/deflate.c:293)
>   }
> }
> 
> Live or dead {
>   564 blocks in heap block record 7 of 1,524
>   36,962,304 bytes (36,962,304 requested / 0 slop)
>   Individual block sizes: 65,536 x 564
>   3.33% of the heap (29.96% cumulative)
>   Allocated at {
>     #01: MOZ_Z_deflateInit2_ (/home/njn/moz/mi4/co64dmd/modules/zlib/src/../../../../modules/zlib/src/deflate.c:302)
>   }
> }
> 
> Live or dead {
>   564 blocks in heap block record 46 of 1,524
>   4,620,288 bytes (3,347,904 requested / 1,272,384 slop)
>   Individual block sizes: 8,192 x 564
>   0.42% of the heap (76.21% cumulative)
>   Allocated at {
>     #01: MOZ_Z_deflateInit2_ (/home/njn/moz/mi4/co64dmd/modules/zlib/src/../../../../modules/zlib/src/deflate.c:277)
>   }
> }

With the patch:

> Cumulative {
>   74 blocks in heap block record 29 of 1,417
>   4,849,664 bytes (4,849,664 requested / 0 slop)
>   Individual block sizes: 65,536 x 74
>   0.69% of the heap (66.74% cumulative)
>   Allocated at {
>     #01: MOZ_Z_deflateInit2_ (/home/njn/moz/mi4/o64dmd/modules/zlib/src/../../../../modules/zlib/src/deflate.c:293)
>   }
> }
> 
> Cumulative {
>   74 blocks in heap block record 30 of 1,417
>   4,849,664 bytes (4,849,664 requested / 0 slop)
>   Individual block sizes: 65,536 x 74
>   0.69% of the heap (67.43% cumulative)
>   Allocated at {
>     #01: MOZ_Z_deflateInit2_ (/home/njn/moz/mi4/o64dmd/modules/zlib/src/../../../../modules/zlib/src/deflate.c:302)
>   }
> }
> 
> Cumulative {
>   74 blocks in heap block record 31 of 1,417
>   4,849,664 bytes (4,849,664 requested / 0 slop)
>   Individual block sizes: 65,536 x 74
>   0.69% of the heap (68.12% cumulative)
>   Allocated at {
>     #01: MOZ_Z_deflateInit2_ (/home/njn/moz/mi4/o64dmd/modules/zlib/src/../../../../modules/zlib/src/deflate.c:299)
>   }
> }
> 
> Cumulative {
>   74 blocks in heap block record 32 of 1,417
>   4,849,664 bytes (4,849,664 requested / 0 slop)
>   Individual block sizes: 65,536 x 74
>   0.69% of the heap (68.81% cumulative)
>   Allocated at {
>     #01: MOZ_Z_deflateInit2_ (/home/njn/moz/mi4/o64dmd/modules/zlib/src/../../../../modules/zlib/src/deflate.c:294)
>   }
> }
> 
> Cumulative {
>   74 blocks in heap block record 118 of 1,417
>   606,208 bytes (439,264 requested / 166,944 slop)
>   Individual block sizes: 8,192 x 74
>   0.09% of the heap (89.19% cumulative)
>   Allocated at {
>     #01: MOZ_Z_deflateInit2_ (/home/njn/moz/mi4/o64dmd/modules/zlib/src/../../../../modules/zlib/src/deflate.c:277)
>   }
> }

In short, we drop from 564 initializations to 74.

Comment 3

[:Benjamin Peterson]

Attachment: share-zlib.patch

Alright, want to take the review, too, or shall we enlist someone else?

Comment 4

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Comment on attachment 8534765 [details] [diff] [review]
share-zlib.patch

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

I'm no expert on this code but this change looks straightforward enough.

::: js/src/vm/Compression.cpp
@@ +41,4 @@
>  }
>  
>  bool
> +Compressor::prepare(const unsigned char *inp_, size_t inplen_)

Nit: Why the trailing underscores?

::: js/src/vm/HelperThreads.h
@@ +310,5 @@
> +    /* 
> +     * Cached compressor that is used for servicing SourceCompressionTasks on
> +     * this thread.
> +     */
> +    Compressor sourceCompressor;

Nit: I found the "Cached" a little confusing. I think the comment would be clearer with that word removed.

Comment 5

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> Comment on attachment 8534765 [details] [diff] [review]
> share-zlib.patch
> 
> Review of attachment 8534765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm no expert on this code but this change looks straightforward enough.
> 
> ::: js/src/vm/Compression.cpp
> @@ +41,4 @@
> >  }
> >  
> >  bool
> > +Compressor::prepare(const unsigned char *inp_, size_t inplen_)
> 
> Nit: Why the trailing underscores?

It keeps the parameters from aliasing class members. Is that still the preferred way or should I use this-> in the method?

> 
> ::: js/src/vm/HelperThreads.h
> @@ +310,5 @@
> > +    /* 
> > +     * Cached compressor that is used for servicing SourceCompressionTasks on
> > +     * this thread.
> > +     */
> > +    Compressor sourceCompressor;
> 
> Nit: I found the "Cached" a little confusing. I think the comment would be
> clearer with that word removed.

Sure

Comment 6

[Nicholas Nethercote [:njn] (On Long Service Leave)]

> > Nit: Why the trailing underscores?
> 
> It keeps the parameters from aliasing class members. Is that still the
> preferred way or should I use this-> in the method?

Trailing underscores for class members is more standard within Spidermonkey, AIUI. Switch where the underscores go if you want, but I'm not too fussed by this.

Comment 7

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/364d6f79d87c

Comment 8

[Nigel Babu [:nigelb]]

I think there's a non-unified bustage from this push. See https://treeherder.mozilla.org/logviewer.html#?job_id=4540530&repo=mozilla-inbound

Comment 9

[Nigel Babu [:nigelb]]

And it looks like, a new Valgrind failure too https://treeherder.mozilla.org/logviewer.html#?job_id=4540401&repo=mozilla-inbound

Comment 10

[Carsten Book [:Tomcat]]

sorry had to back this out as nigel mentioned for test failures in valgrind and non-unified bustage 
https://treeherder.mozilla.org/logviewer.html#?job_id=4540401&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=4540530&repo=mozilla-inbound

Comment 11

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10692972a7b6

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/10692972a7b6

Comment 13

[Nicholas Nethercote [:njn] (On Long Service Leave)]

This ended up being backed out in bug 1111564 because it caused a 3 MiB RSS regression on Talos, and the benefits of the reduced churn were unclear.