Closed
Bug 620833
Opened 14 years ago
Closed 13 years ago
TM: Release infrequently used traces
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: billm, Unassigned)
Details
(Keywords: memory-footprint, Whiteboard: [cib-memory])
Attachments
(1 file, 1 obsolete file)
6.13 KB,
patch
|
Details | Diff | Splinter Review |
When bug 584860 lands, we will have a 16 MB trace cache in each compartment. This makes it more important that we release the code that isn't being used. We should be able to adopt the same scheme as bug 617656 did for the method JIT.
Reporter | ||
Updated•14 years ago
|
Assignee: general → wmccloskey
blocking2.0: --- → ?
Updated•14 years ago
|
blocking2.0: ? → betaN+
Updated•14 years ago
|
Whiteboard: [cib-memory]
Reporter | ||
Comment 1•14 years ago
|
||
I looked this over a bit. Although Nanojit has a fancy-looking code allocator, it doesn't actually release memory to the OS until you call reset(). Since I'd rather not do any serious Nanojit hacking, we're going to have to release memory at the granularity of an entire compartment. However, I also realized that we have an advantage. Unlike in the methodjit, it's pretty easy to keep a timestamp of the last time we ran some in a given compartment. We'd just have to add a PRMJ_Now call to ExecuteTree. Then we could either implement some kind of LRU scheme, or we could just flush any compartment during GC that hasn't executed a trace in 5 seconds. Does this sound okay?
![]() |
||
Comment 2•14 years ago
|
||
(In reply to comment #1) > I looked this over a bit. Although Nanojit has a fancy-looking code allocator, > it doesn't actually release memory to the OS until you call reset(). Yeah... comment from jscntxt.h: * - dataAlloc has the lifecycle of the monitor. It's flushed only when * the monitor is flushed. It's used for fragments. * * - codeAlloc has the same lifetime as dataAlloc, but its API is * different (CodeAlloc vs. VMAllocator). It's used for native code. * It's also a good idea to keep code and data separate to avoid I-cache * vs. D-cache issues. > Since I'd > rather not do any serious Nanojit hacking, we're going to have to release > memory at the granularity of an entire compartment. I'm happy to do any such hacking if you can tell me what you need. Nanojit allocates buffers for native code in 64KB chunks.
Reporter | ||
Comment 3•14 years ago
|
||
This patch does two things. First, it causes us to flush any TraceMonitor that hasn't run a trace in the last 5 seconds; this happens during GC. Second, it flushes the math cache every GC. The heuristic here is fairly aggressive. However, traces can be compiled pretty quickly, so it seems reasonable to me. I wasn't able to see any performance impact in browser or in the shell. I measured memory usage while browsing. Without the patch, I was able to get the trace cache to use about 2.5MB of memory. With the patch, memory usage never went over ~500KB. I actually found it pretty hard to make memory usage go really high during normal browsing. It seems like we flush the trace cache often enough without this patch that it never gets too bad. This patch really just ensures that we never hit worst-case behavior.
Attachment #499901 -
Flags: review?(gal)
Updated•14 years ago
|
Whiteboard: [cib-memory] → [cib-memory][softblocker]
Reporter | ||
Comment 4•14 years ago
|
||
The graph here: http://graphs.mozilla.org/#tests=[[29,1,598]] shows that the max heap size increased when bug 584860 landed. That gives evidence that it's important to fix this bug, and it allows us to verify whether we've actually fixed it.
Updated•14 years ago
|
Attachment #499901 -
Flags: review?(gal) → review+
![]() |
||
Comment 5•14 years ago
|
||
Have you tried numbers other than 5 seconds? It would be nice to know that this arbitrary number is better/worse than other arbitrary numbers.
Reporter | ||
Comment 6•14 years ago
|
||
(In reply to comment #5) > Have you tried numbers other than 5 seconds? It would be nice to know that > this arbitrary number is better/worse than other arbitrary numbers. That would be nice. I wish I had a better way to test this patch. Nothing I thought of visiting actually used that much memory. Do you know of any pages that allocate a ton of traces without flushing?
![]() |
||
Comment 7•14 years ago
|
||
(In reply to comment #6) > (In reply to comment #5) > > Have you tried numbers other than 5 seconds? It would be nice to know that > > this arbitrary number is better/worse than other arbitrary numbers. > > That would be nice. I wish I had a better way to test this patch. Nothing I > thought of visiting actually used that much memory. Do you know of any pages > that allocate a ton of traces without flushing? No. This raises the question: how important is this change? We know that flushing the methodJIT is important because it generates code like a drunken sailor, but do we have any evidence that the traceJIT does the same?
Reporter | ||
Comment 8•14 years ago
|
||
(In reply to comment #7) > No. This raises the question: how important is this change? We know that > flushing the methodJIT is important because it generates code like a drunken > sailor, but do we have any evidence that the traceJIT does the same? Yeah, I was thinking that myself. However, the graph in comment 4 suggests that something bad can potentially happen. I guess I just need to replicate that test :-).
![]() |
||
Comment 9•14 years ago
|
||
Looks like bug 623428 may be the cause of the comment 4 increase.
Comment 10•14 years ago
|
||
(In reply to comment #9) > Looks like bug 623428 may be the cause of the comment 4 increase. If that is true, can this bug be closed?
Reporter | ||
Comment 11•14 years ago
|
||
(In reply to comment #10) > (In reply to comment #9) > > Looks like bug 623428 may be the cause of the comment 4 increase. > > If that is true, can this bug be closed? Well, no. We still have a 16MB trace cache per tab. Most of the time we don't use nearly that much, but it does seem a little worrisome. I'm not sure if this should still block, though.
Updated•14 years ago
|
blocking2.0: betaN+ → -
Updated•14 years ago
|
Whiteboard: [cib-memory][softblocker] → [cib-memory]
Reporter | ||
Comment 12•14 years ago
|
||
This probably won't land now, but I want to post the final patch in case we decide to do it later.
Attachment #499901 -
Attachment is obsolete: true
Reporter | ||
Updated•13 years ago
|
Assignee: wmccloskey → general
Comment 13•13 years ago
|
||
Obsolete with the removal of tracejit.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•