Closed Bug 1084177 Opened 5 years ago Closed 5 years ago

Reduce heap churn caused by JS source compression

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: njn, Assigned: Benjamin)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
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.
Attachment #8534349 - Flags: feedback?(n.nethercote)
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.
Attachment #8534349 - Flags: feedback?(n.nethercote) → feedback+
Attached patch share-zlib.patchSplinter Review
Alright, want to take the review, too, or shall we enlist someone else?
Attachment #8534349 - Attachment is obsolete: true
Attachment #8534765 - Flags: review?(n.nethercote)
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.
Attachment #8534765 - Flags: review?(n.nethercote) → review+
(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
> > 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.
I think there's a non-unified bustage from this push. See https://treeherder.mozilla.org/logviewer.html#?job_id=4540530&repo=mozilla-inbound
https://hg.mozilla.org/mozilla-central/rev/10692972a7b6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1111564
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.
You need to log in before you can comment on or make changes to this bug.