High LUL memory usage with the profiler and Fission
Categories
(Core :: Gecko Profiler, defect, P2)
Tracking
()
People
(Reporter: yoasif, Assigned: jseward)
References
Details
Attachments
(3 files)
(deleted),
application/gzip
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Noticed Firefox taking up a lot of memory on my Linux machine. I am running a bunch of extensions in Nightly, but this is unusual.
Took a memory report in the hopes that we can investigate this further.
Thanks!
Reporter | ||
Comment 1•4 years ago
|
||
Comment 2•4 years ago
|
||
Andrew, could you please take a look over the memory report?
Thanks!
Comment 3•4 years ago
|
||
Thanks for the report. It looks to me that the largest issue is that you have around 300MB or so of memory being used in every content process for the profiler. Because you have Fission enabled, there are a lot of processes.
Here's an excerpt from a typical process:
345.05 MB (100.0%) -- explicit
├──322.71 MB (93.53%) -- profiler
│ ├──211.21 MB (61.21%) ── lul
│ └──111.50 MB (32.32%) ── profiler-state
This makes bug 1635442 more important now.
Comment 5•4 years ago
|
||
Julian, can we place lul data in shared memory? kmag says lul data is duplicated in each content process. Fission site isolation can use many more content processes than e10s, so profiler memory usage is a problem with Fission.
This bug doesn't need to block Fission Nightly. Tracking for Fission riding the trains to Beta (M7).
Comment 6•4 years ago
|
||
That's one option. Bug 1635811 proposes another option: caching the lul data in a file. "[...] If we map the same cache file into each process, this would also save memory."
Assignee | ||
Comment 7•4 years ago
|
||
I should point out that (unless it has been changed since the initial implementation), the way that the unwind information is stored in LUL is grossly space-inefficient. We could do very much better. I wrote a blog post [1] about it, back in the day. So maybe implementing that would help too.
Assignee | ||
Comment 8•4 years ago
|
||
But to answer your question directly: the lul data is readonly (once it has been read from the .so file, and we're in "unwinding" mode), so in that aspect there's no problem in putting it in shared memory. However, the same .so file will normally be mapped at different virtual addresses in different processes. And I can't remember offhand whether the lul data is phrased in terms of offsets from the start of .so's, or whether it has the absolute virtual addresses baked in. In the latter case, it would then not be sharable across processes. I'd have to look.
However, even in the latter case, it would be pretty simply to refactor the info to use the offset-based scheme.
All of which is a long way of saying: yes we should be able to share it across processes; there may (or may not) be a need to do a bit of refactoring though.
Left to myself, though, I would first follow the line of enquiry suggested in comment 7. IMO there are huge wins there and they are easy to do. Large fruit hanging very low, etc.
Comment 9•4 years ago
|
||
(In reply to Julian Seward [:jseward] from comment #8)
All of which is a long way of saying: yes we should be able to share it across processes; there may (or may not) be a need to do a bit of refactoring though.
Left to myself, though, I would first follow the line of enquiry suggested in comment 7. IMO there are huge wins there and they are easy to do. Large fruit hanging very low, etc.
Thanks. Summary of the three suggestions so far:
- optimize unwind info to be more space efficient (comment 7)
- map unwind info in parent process' shared memory (comment 0)
- cache unwind info in a file (comment 6, pointing to bug 1635811), though comments suggest content process sandboxing might make sharing the cache file difficult.
Comment 10•4 years ago
|
||
And
- Don't initialize LUL until somebody starts profiling (comment 4).
Comment 11•4 years ago
|
||
Wait, I guess we're already doing that. Bug 1635442 is just asking for a way to do profiling without LUL (and without stacks). Never mind then.
Comment 12•4 years ago
|
||
Assigning to Julian. He said he is available to help reduce LUL memory usage for one week in Q3 and one week in Q4.
He will try to optimize unwind info to be more space efficient (the approach suggested in comment 7), not map unwind info in parent process' shared memory (comment 0) or a file (comment 6). If profiler memory usage is still a problem after he optimizes unwind info, we can then look into also using shared memory.
Assignee | ||
Comment 13•4 years ago
|
||
Here's my current WIP patch. It's pretty complete, I think, but currently I have only tested it on x86_64-linux. It reduces the memory consumption of the LUL component, as shown in about:memory, by almost a factor of 6 (so, here, from 211 MB down to about 35.2 MB) and can fairly reliably be assumed to do the same independent of the particular build flags etc used. Feedback welcomed.
Next week I'll test it on other platforms and try to get something reviewable available.
Assignee | ||
Comment 14•4 years ago
|
||
Patch moved to bug 1665029.
Comment 15•4 years ago
|
||
Asif, do you still Nightly taking up lots more memory with Fission than without Fission? Were you using the Firefox Profiler when you saved your memory report?
A memory usage optimization for Firefox Profiler's memory usage (bug 1665029) landed last week.
Reporter | ||
Comment 16•4 years ago
|
||
Chris, I may have been running the profiler just to get a handle on what was happening (nothing really visible). Unfortunately, Firefox crashed very soon after I attempted to capture the profile (I think it may have been the OOM-killer in Linux).
I haven't seen some of the extremely bad behavior in a few days, but I am not sure if it is a coincidence. I'm still running with Fission enabled though, and will be happy to share another memory report if it seems bad (along with a profile, if I can manage to capture it).
Comment 17•4 years ago
|
||
Closing as WORKSFORME. I assume profiler memory usage is good enough now that Julian's improvements in bug 1665029 have landed. If not, please file a new bug.
Updated•4 years ago
|
Description
•