Bill McCloskey [inactive unless it's an emergency] (:billm) Reporter
	Description • 14 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm) 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?

	Nicholas Nethercote [inactive]
	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.

	Bill McCloskey [inactive unless it's an emergency] (:billm) 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.

	Bill McCloskey [inactive unless it's an emergency] (:billm) 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.

	Nicholas Nethercote [inactive]
	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.

	Bill McCloskey [inactive unless it's an emergency] (:billm) 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?

	Nicholas Nethercote [inactive]
	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?

	Bill McCloskey [inactive unless it's an emergency] (:billm) 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 :-).

	Nicholas Nethercote [inactive]
	Comment 9 • 14 years ago

Looks like bug 623428 may be the cause of the comment 4 increase.

	Paul Wright
	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?

	Bill McCloskey [inactive unless it's an emergency] (:billm) 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.

	Bill McCloskey [inactive unless it's an emergency] (:billm) 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 13 years ago

Obsolete with the removal of tracejit.