Closed
Bug 834936
Opened 12 years ago
Closed 12 years ago
Temporarily preprocess Health Report JSMs into a single file to reduce compartment overhead
Categories
(Firefox Health Report Graveyard :: Client: Desktop, defect)
Firefox Health Report Graveyard
Client: Desktop
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 21
People
(Reporter: gps, Assigned: gps)
References
Details
(Whiteboard: [MemShrink])
Attachments
(1 file)
(deleted),
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•12 years ago
|
||
CC'ing billm so he sees 218% overhead for using multiple compartments in this scenario.
Updated•12 years ago
|
Whiteboard: [MemShrink]
Comment 2•12 years ago
|
||
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+
Updated•12 years ago
|
Flags: needinfo?(wmccloskey)
Summary: Combine JSMs into fewer compartments → Temporarily preprocess Health Report JSMs into a single file to reduce compartment overhead
Assignee | ||
Comment 3•12 years ago
|
||
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)
Assignee | ||
Comment 5•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink][fixed in services] → [MemShrink]
Target Milestone: --- → mozilla21
Comment 6•12 years ago
|
||
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 :(
Updated•12 years ago
|
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla21 → Firefox 21
Updated•6 years ago
|
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•