Closed Bug 831397 Opened 12 years ago Closed 12 years ago

talos reported 5.6% maxheap regression on fhr landing

Categories

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

Product:
Firefox Health Report Graveyard
Component:
Client: Desktop
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WORKSFORME

People

(Reporter: taras.mozilla, Unassigned)

References

Details

(Whiteboard: [MemShrink:P2])

From that email for OS X 10.7: Prev avg 52332710.000 New avg 55257100.000 Delta: 2,924,390 Looking at my about:memory on latest OS X Nightly, we have somewhere between 1 and 1.5MB in JSMs corresponding to new code landed for this feature. I don't see us reducing that significantly. We might be able to shave off a few hundred by combining JSMs into single files thus eliminating JS compartment overhead. Yay CPG. I also see ~500K for the healthreport.sqlite database. I'm not sure how much control we have over that. And, I recall seeing this sitting >1MB on my Windows machine at home. Not sure what the deal is. So, I've accounted for ~2MB in memory overhead. Not sure where the other 1MB is coming from. New features will incur extra memory usage. Is 3MB too much? How do you propose I lower it?
> New features will incur extra memory usage. I totally agree if we s/will/may/. > Is 3MB too much? It's obviously a cost/benefit analysis wrt the feature. 3mb isn't a lot in itself, but of course a few features all taking up 3mb add up. > How do you propose I lower it? Of course Taras is probably not the right person to answer that question. How often does the user interact with FHR? Perhaps we could unload parts of it when the user is not interacting with it. Also, perhaps FHR doesn't need a .5mb database loaded in memory at all times?
(In reply to Justin Lebar [:jlebar] from comment #2) > > New features will incur extra memory usage. > > I totally agree if we s/will/may/. > > > Is 3MB too much? > > It's obviously a cost/benefit analysis wrt the feature. 3mb isn't a lot in > itself, but of course a few features all taking up 3mb add up. > I'm glad you can account for the 3mb increase, makes optimization easier. I think 3mb is a lot for an always-on lightweight stats-collection mechanism. This cancels out some hard work that went into lowering our footprint on android/b2g. I suspect we can improve that without reducing functionality of FHR. We've run into similar issues in the past. Possible solutions tried elsewhere: keeping db closed, refactoring your code to only keep a small part of the codebase loaded during normal operation(resource usage would increase during serialization).
(In reply to Taras Glek (:taras) from comment #3) > I'm glad you can account for the 3mb increase, makes optimization easier. I > think 3mb is a lot for an always-on lightweight stats-collection mechanism. > This cancels out some hard work that went into lowering our footprint on > android/b2g. What is too much? Also, B2G is not using CPG (AFAIK), so the ridiculous per-compartment overhead (which I'm estimating would be ~1MB here) would go away. I proposed a solution in bug 807205 to work around the CPG issue until the overhead problem is fixed. I would love it for the Perf Team to evaluate that bug. > Possible solutions tried elsewhere: > keeping db closed, Then we have increased load when writing. This would also worsen the "need to flush on shutdown" problem. > refactoring your code to only keep a small part of the > codebase loaded during normal operation(resource usage would increase during > serialization). How is this done? My understanding is once you Cu.import(), the module lingers forever.
> Also, B2G is not using CPG (AFAIK), so the ridiculous per-compartment overhead (which I'm > estimating would be ~1MB here) would go away. Our b2g devices have ~100mb of usable RAM available to us. A 1mb improvement is the class of bugs that we'd consider worth investing time in. The database alone is 500kb here, aiui. (FYI, b2g does use CPG, but shoves many system components into one compartment. I don't know how that affects your code here, but I suspect it would probably go into the same compartment.)
Android will soon be using an out-of-band uploader, which will avoid loading a chunk of Gecko JS. We might even get away with eliminating serialization, too.
> Also, B2G is not using CPG (AFAIK), so the ridiculous per-compartment > overhead (which I'm estimating would be ~1MB here) would go away. The per-compartment overhead is nothing like 1 MB. On my 64-bit machine, really tiny compartments that have almost nothing in them take up roughly 30--40 KiB. That's more than we'd like, but let's not exaggerate.
(In reply to Nicholas Nethercote [:njn] from comment #7) > The per-compartment overhead is nothing like 1 MB. On my 64-bit machine, > really tiny compartments that have almost nothing in them take up roughly > 30--40 KiB. That's more than we'd like, but let's not exaggerate. I think Greg is referring to the additional overhead of cross-compartment references, not the overhead of an individual compartment. See Bug 807205.
Which platforms (desktop, android, b2g) is FHR enabled on? There are definitely some improvements to be made on the JS side, but if there are some easy wins to be had in the meantime that would be great.
Whiteboard: [MemShrink] → [MemShrink:P2]
(In reply to Nicholas Nethercote [:njn] from comment #9) > Which platforms (desktop, android, b2g) is FHR enabled on? Just desktop. Android will be coming in a few weeks, hopefully. As for reducing memory usage, I see the following options: 1) Periodically clear out SQLite caches. This should reduce the SQLite database connection overhead to ~75k. 2) Merge JSMs.
3) If it were feasible to unload JSMs that you're not using, would you be able to take advantage of that for FHR, or is FHR always doing stuff?
(In reply to Justin Lebar [:jlebar] from comment #11) > 3) If it were feasible to unload JSMs that you're not using, would you be > able to take advantage of that for FHR, or is FHR always doing stuff? FHR is always doing stuff. About the only part we could offload would be uploading. And, that would require splitting uploading into its own JSM. As it turns out, we'll likely need to do this to facilitate Android. As I stated in my dev-platform email on the effects of JSMs, the current situation sucks. I can reduce maybe 500k from the regression numbers by cleaning SQLite. The rest will require merging JSMs. I'd like to petition people to accept 2.5-3MB as a necessary tradeoff for the valuable data FHR will provide. If 2.5MB is not acceptable, what is?
> I'd like to petition people to accept 2.5-3MB as a necessary tradeoff for the valuable data FHR > will provide. If 2.5MB is not acceptable, what is? I don't want to mischaracterize anyone else's feelings, but I think you are/were probably the target of general negative feelings against new JS components being developed and adding to our memory usage. I don't want to speak ill of anyone's projects in particular, but I'm certain you can think of a handful of Firefox components which do nothing for you but suck up memory. (You and I may not have the same list, of course.) There's been a history of these sort of features being written with little regard for their memory usage and then essentially abandoned. Later, when they hit some edge case where they use a ton of memory, we in MemShrink end up cleaning up the mess. I don't mean to suggest that yours is this sort of feature, nor that you're this sort of developer. On the contrary, you seem to be on top of this stuff. But we've been burned. OTOH we have some much more serious memory regressions on our hands right now, and 2.5mb -- if that's really all this is -- is going to pass well under our radar. We should make sure njn is OK with this, but speaking for myself, if in your assessment you've done everything reasonable to reduce FHR's memory usage, and if you commit to addressing bugs where FHR's memory usage exceeds its envelope, I don't think we have a serious objection from a MemShrink POV.
> We should make sure njn is OK with this, but speaking for > myself, if in your assessment you've done everything reasonable to reduce > FHR's memory usage, and if you commit to addressing bugs where FHR's memory > usage exceeds its envelope, I don't think we have a serious objection from a > MemShrink POV. Sounds ok to me.
We've slowly been chipping away at this. We were originally staring at ~2.9MB increase. I believe we are now around 0.9-1.2MB. Most of the savings came from using preprocessor magic to merge JSM files together so we have 2 compartments instead of like 10 (bug 834936 and bug 836177). I think this freed ~1.5MB. I can't say it too many times: zones can't come soon enough. The other big chunk was purging memory from the SQLite connection at opportune times. That freed say 400k. What's left? We can still get 80k from closing the SQLite connection when it isn't be used (at the expense of higher latency and I/O when we do write to it - this will bite us later when we perform more DB writes). One local profile run showed that not instantiating the FHR probes saved ~133k. I'm not sure why this is. I filed bug 836186 to investigate that. After that, I honestly don't see much to optimize. Zones should reclaim some more compartment inefficiencies. After that, well, we might find a few one-time leaks or sub-optimal allocations in JS. But, I'm relatively confident it will be the point of diminishing returns and the next big gain would come from writing the feature in C++. And, I'm not sure chasing a few hundred kb is worth the overhead of writing a feature in C++ instead of JS. Time will tell. Until then, I'll keep chasing memory.
> We've slowly been chipping away at this. We were originally staring at > ~2.9MB increase. I believe we are now around 0.9-1.2MB. Nice. I'm pretty happy with this -- ~1 MB isn't much for desktop Firefox, which is the only product that FHR is present in at the moment. When it comes to Fennec's memory consumption, kats is the expert. ~1 MB might be a tougher sell there. Hopefully the zones work will land before then, and maybe some other ideas might come along in the meantime.
I don't have anything much to say beyond what has been said already in comments 13 and 14. You've clearly put in some effort into (and gotten good results from) reducing memory usage, and I'm fine with this going into Fennec. A 1 MB hit will probably be noticeable on some of our AWSY metrics but it's good to know it's going towards something useful.
> I'm pretty happy with this. I know it's bad bugzilla etiquette to say this, but: Me too. Thanks, gps.
FWIW, I did an A/B push with FHR disabled and enabled. Enabled: https://tbpl.mozilla.org/?tree=Try&rev=662ff72365bd Disabled: https://tbpl.mozilla.org/?tree=Try&rev=a7d5cdce3b28 http://perf.snarkfest.net/compare-talos/index.html?oldRevs=a7d5cdce3b28&newRev=662ff72365bd&submit=true When you factor out the Android tests (I accidentally ran them and the results are noise) and account for variance in results, I think the only regression we have is memory. Please note the numbers are higher (2MB range) than what was previously stated in this bug because since I last reported the 1MB figure, I cleaned up Sync's memory usage and JSMs previously shared by FHR and Sync are now used exclusively by FHR. This means that the memory associated with these compartments is now attributed to FHR, not Sync. njn: Does this bug have any additional value or should we close?
Flags: needinfo?(n.nethercote)
Can we link the appropriate bugs to the depends/blocks lists here? I linked some mentioned above but I think there might be more.
Blocks: 828152
Depends on: 834936, 836177, 836186
I'm ok with closing this bug -- you've reduced the memory consumption significantly, and what remains seems reasonable for this feature. Kats, are you happy if it's closed?
Flags: needinfo?(n.nethercote)
Yup, sounds good to me. Thanks for digging into this, gps!
I guess WORKSFORME is the appropriate resolution given the previous 2 comments.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.