Closed Bug 834936 Opened 11 years ago Closed 11 years ago

Temporarily preprocess Health Report JSMs into a single file to reduce compartment overhead

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 21

People

(Reporter: gps, Assigned: gps)

References

Details

(Whiteboard: [MemShrink])

Attachments

(1 file)

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.
Attachment #706631 - Flags: review?(rnewman)
CC'ing billm so he sees 218% overhead for using multiple compartments in this scenario.
Whiteboard: [MemShrink]
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.
Attachment #706631 - Flags: review?(rnewman) → review+
Flags: needinfo?(wmccloskey)
Summary: Combine JSMs into fewer compartments → Temporarily preprocess Health Report JSMs into a single file to reduce compartment overhead
https://hg.mozilla.org/services/services-central/rev/b0d6d514d341

With semicolons and changed commit message.
Whiteboard: [MemShrink] → [MemShrink][fixed in services]
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.
Flags: needinfo?(wmccloskey)
https://hg.mozilla.org/mozilla-central/rev/b0d6d514d341
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink][fixed in services] → [MemShrink]
Target Milestone: --- → mozilla21
Blocks: 834041
Blocks: 836177
Blocks: 836285
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 :(
Blocks: 837803
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla21 → Firefox 21
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: