[Webkit-unassigned] [Bug 85063] Add low memory check in ExecutableAllocator::underMemoryPressure()

[bugzilla-daemon at webkit.org]

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





--- Comment #30 from Filip Pizlo <fpizlo at apple.com>  2012-05-08 16:47:01 PST ---
(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > (In reply to comment #24)
> > > > (From update of attachment 139452 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=139452&action=review
> > > > 
> > > > > Source/JavaScriptCore/ChangeLog:9
> > > > > +        Call WTF::isSystemMemoryLow() to check whether system memory is low in
> > > > > +        ExecutableAllocator::underMemoryPressure(). This will force collecting
> > > > > +        of garbage, thus avoiding out of system memory while at the same time
> > > > > +        we have a lot of unused memory waiting to be released.
> > > > 
> > > > I'm not sure I buy this.  ExecutableAllocator has _nothing_ to do with garbage collection.  It only deals with JIT memory.  This method returning true should only cause us to jettison JIT memory; it will not cause any other kinds of memory reclamation.
> > > 
> > > Can't this be considered useful?  Seems to me it is.
> > 
> > That's besides the point.  The change log comment is wrong.
> 
> Ok, it wasn't clear to any of the three of us that this was your point.  
> 
> Lyon, you should fix that.
> 
> Any other concerns with it Filip?

I think that this is an unusual and incomplete policy, particularly since now that we have the LLInt, it is going to be tempting to get rid of underMemoryPressure() altogether.

This policy means that if "system memory is low" [sic], you will force recompilation of all JS code, every time you enter JS execution.

One problem is that you're created an undocumented, and likely Blackberry-specific, notion of what it means for memory to be low.  The normal meaning of underMemoryPressure() is that you've used up half your quota of executable memory, which ought to never happen, since with the LLInt you're already using hyperbolic heuristics for the JIT that should ensure that the probability of getting to 1/2 is tiny.

Another problem is that you're conflating overall system memory with JIT memory.  What if you are actually using almost zero JIT memory but what little memory you are using belongs to frequently executed code?  That is a common case, and you're completely shafting performance for that case. You're also shafting memory use, since it takes more memory to compile code than it does to keep it around. Note that since we typically run JSC with a fixed pool of JIT memory - which means that we have a fixed quota of how much JIT memory we plan to use - we can ensure that if all executable memory is jettisoned on one entry into JS code, then it is highly unlikely to be jettisoned again the next time, since by definition a jettisoning will bring us back below our quota. But since you're conflating JIT memory use with memory use in general, it may be that on _every_ entry into JS code you will jettison _all_ JS executable code because the rest of the system thinks it's under pressure, an
 d then you'll use possibly more memory than what you freed since compilation is expensive.

Finally on many platforms there is unlikely to be a nice and tidy notion of system memory being low.  Mobile/embedded platforms might have this notions, but desktops don't - you've always got more virtual memory, and the effect of using more of it is steady degradation of performance through the steady increase in paging. So there is no discrete low memory state.

On the other hand, I can appreciate the need for these tweaks in shipping software, and if the Blackberry port finds it useful, then having it in the tree, with appropriate guards for Blackberry, is probably great for everyone. In that case I would remove the SystemMemoryStatus.h header and place all the code inside underMemoryPressure(). Then it becomes clear what you're doing: you're instituting a Blackberry-specific policy that links whatever it is that BlackBerry::Platform::isMemoryLow() does to JSC's notion of underMemoryPressure().

This is particularly useful if in the future we decide to remove underMemoryPressure() and introduce some more rational reasoning about how much JIT memory ought to be used at any time - for example using a least-recently-used jettisoning of executable code. If that were to happen, having the code in one place and not abstracted behind an underspecified WTF API will help us reason about how to change the code in a way that does not completely break Blackberry's heuristics.

In short, I appreciate the need for this patch but would recommend (1) getting rid of SystemMemoryStatus.h and simply having the code inline in underMemoryPressure() and (2) adding some comments about what you actually want the heuristics to be. That minimizes the risk of the code succumbing to bit rot, and it also ensures that other ports don't attempt to do similar things but without the benefit of the Blackberry-specific memory pressure APIs.

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