Last Comment Bug 834936 - Temporarily preprocess Health Report JSMs into a single file to reduce compartment overhead
: Temporarily preprocess Health Report JSMs into a single file to reduce compar...
Status: RESOLVED FIXED
[MemShrink]
:
Product: Firefox Health Report
Classification: Client Software
Component: Client: Desktop (show other bugs)
: unspecified
: All All
: -- normal
: Firefox 21
Assigned To: Gregory Szorc [:gps]
:
:
Mentors:
Depends on:
Blocks: 836285 828152 831397 834041 836177 837803
  Show dependency treegraph
 
Reported: 2013-01-25 15:22 PST by Gregory Szorc [:gps]
Modified: 2013-04-08 08:35 PDT (History)
12 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Merge JSM with preprocessor magic, v1 (22.14 KB, patch)
2013-01-25 15:22 PST, Gregory Szorc [:gps]
rnewman: review+
Details | Diff | Splinter Review

Description Gregory Szorc [:gps] 2013-01-25 15:22:25 PST
Created attachment 706631 [details] [diff] [review]
Merge JSM with preprocessor magic, v1

A significant portion of FHR's memory usage is due to compartment overhead clown shoes.

The attached patch employs some preprocessor magic to combine JSMs together into monolithic output files so they are loaded into the same compartment.

Instead of about 10 compartments, we now have 2. We have a compartment for the data reporting XPCOM service. And, we have a compartment for most of the FHR code.

On the set of compartments that I merged:

Before: 1415 kb
After:   649 kb
Savings: 766 kb

I could roll a few more services-common modules into the large compartments. But, these modules are shared with Sync and some parts of browser UX, so we'd be introducing some redundancy. There is also the case to be made that JS zones will land soon and this effort isn't worth it.

Anyway, I think I found a good balance between small and simple patch and net gain. After this patch and the recently-landed SQLite optimizations, FHR now comes in at 730k on my builds. Hopefully the additional compartment/zone work billm is doing will reduce this further.

I fully intend for this patch to be temporary until compartments don't have so much waste.
Comment 1 Gregory Szorc [:gps] 2013-01-25 15:30:03 PST
CC'ing billm so he sees 218% overhead for using multiple compartments in this scenario.
Comment 2 Richard Newman [:rnewman] 2013-01-25 15:58:41 PST
Comment on attachment 706631 [details] [diff] [review]
Merge JSM with preprocessor magic, v1

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

Code-wise, this seems like the least painful approach.

(Though you should probably change the commit message and the bug title to something like "Temporarily preprocess Health Report JSMs into a single file to reduce compartment overhead".)


My only practical concerns here are:

1. That we're no longer running unit tests against the code that we're shipping. That increases our need for manual verification of every step in the process, and increases risk.

2. The same from a perf perspective: one of the purported advantages of compartments is that it changes the scope of GC. We need to figure out whether in saving baseline compartment overhead we are damaging our runtime GC profile, which could harm the user experience.

I'd love to get an estimate from :billm about (a) how much of this is redundant work, given zones, and (b) what the timeframe is on that work.

::: services/metrics/Metrics.jsm
@@ +20,5 @@
> +#define MERGED_COMPARTMENT
> +
> +#include collector.jsm
> +#include dataprovider.jsm
> +#include storage.jsm

For the sake of thoroughness, stick a semicolon between each of these, just to make sure that some crazy JS ASI parsing nonsense doesn't bite.
Comment 3 Gregory Szorc [:gps] 2013-01-27 11:32:53 PST
https://hg.mozilla.org/services/services-central/rev/b0d6d514d341

With semicolons and changed commit message.
Comment 4 Bill McCloskey (:billm) 2013-01-27 12:21:10 PST
I don't know yet whether this is worth it or not after zones. However, it seems worth doing now. I don't want to make any concrete estimate about how long zones will take.

There isn't really any concern about effect on GC heuristics. We never do GCs that collect some JSM compartments and not others.
Comment 5 Gregory Szorc [:gps] 2013-01-28 11:06:05 PST
https://hg.mozilla.org/mozilla-central/rev/b0d6d514d341
Comment 6 Dave Townsend [:mossop] 2013-02-01 15:05:29 PST
This bit me today. If you make a change to any of the files included in Metrics.jsm you have to clobber the build so Metrics.jsm gets updated :(

Note You need to log in before you can comment on or make changes to this bug.