[Webkit-unassigned] [Bug 68329] DFG should support continuous optimization

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=68329





--- Comment #20 from Filip Pizlo <fpizlo at apple.com>  2011-09-20 23:32:12 PST ---
So this discussion uncovered a bug: JettisonedCodeBlocks traces all CodeBlocks it has, not just the ones that were marked.  I'll fix that.

But that observation also uncovers why your two proposals (blowing away JettisonedCodeBlocks on VM exit and/or having a separate mini-GC for JettisonedCodeBlocks) will cause a memory leak.

When a GC happens, we want all dead state to go away, within the limits of conservative scanning.  CodeBlocks can refer to a lot of stuff.  So we only want to scan them if they're live.  That means marking those jettisoned CodeBlocks that are found to be referenced from the RegisterFile.  Your two proposals, as I understand them, won't do this, since they both imply scanning the RegisterFile for CodeBlocks in a separate phase that is not part of the GC.  This means that the GC risks marking too much stuff, and hence, leaking memory.  A memory leak is likely to be less performant than calling JettisonedCodeBlocks::mark() in ConservativeRoots.

I see two options that are robust, and that address some of your concerns:

1) Add a markCodeBlocks(JettisonedCodeBlocks&) method to RegisterFile.  This can have a fast check to see if JettisonedCodeBlocks is empty, in which case it does no work.  This is probably the easiest conceptually, and will probably perform just fine.

2) Add an enum parameter to ConservativeRoots::add() that says if you're looking at a RegisterFile.  This is the best option.  ConservativeRoots already does a linear scan over the RegisterFile.  It's most efficient to have that scan include CodeBlock detection, so we're only touching each location in the RegisterFile once during a collection.  If that enum tells ConservativeRoots::add() that it's looking at the RegisterFile, it can go and look at JettisonedCodeBlocks.  Otherwise it can ignore them.  This parameter could even be a template parameter, or something like that, to ensure that we don't do extra checks when scanning the C stack.

There's a separate question of whether or not to add the bloom filter (or similar) optimizations to JettisonedCodeBlocks.  I don't think that will be necessary.  JettisonedCodeBlocks should be small or empty.  If it's empty then in (1) we can just bail out immediately.  It's more likely to be small, but not empty, though, since the continuous optimization system does seem to want to recompile stuff in roughly half of our benchmarks.  A small set of JettisonedCodeBlocks means a small hashtable, which is likely to be no slower than a bloom filter.

If JettisonedCodeBlocks gets big, then it means that we've already lost, since it could only happen if a lot of recompilation occurred.  My goal is to ensure that this doesn't happen.  This should already be the case, because of the exponential back-off in recompilation that is already part of this patch.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.