Add low memory check in ExecutableAllocator::underMemoryPressure()

• Component: JavaScriptCore
• Status: RESOLVED LATER

[Lyon Chen]

Right now ExecutableAllocator::underMemoryPressure() will always return false, when EXECUTABLE_MEMORY_LIMIT is not defined. Add a call to BlackBerry::Platform::isMemoryLow() to check whether memory is low and return its value.

[Lyon Chen]

Created attachment 139198 [details]
Patch for bug 85063

[Yong Li]

+Filip and Michael

An alternative solution is create a ExecutableAllocatorBlackBerry.cpp, and put our underLowMemory() there. In ExecutableAllocator.cpp, we wrap the function with if !PLATFORM(BLACKBERRY). This pattern is widely used in other files like GraphicsContext.cpp IIRC.

[Antonio Gomes]

(In reply to comment #2)
> +Filip and Michael
> 
> An alternative solution is create a ExecutableAllocatorBlackBerry.cpp, and put our underLowMemory() there. In ExecutableAllocator.cpp, we wrap the function with if !PLATFORM(BLACKBERRY). This pattern is widely used in other files like GraphicsContext.cpp IIRC.

ScrollView also do this.

Or something like:

<agomes> +static bool isOverlimit()
<agomes> +{
<agomes> +#if PLATFORM(BLACKBERRY)
<agomes> +     return BlackBerry::Platform::isMemoryLow();
<agomes> +#endif
<agomes> +
<agomes> +     return false;
<agomes> +}
<agomes> +
<agomes>  bool ExecutableAllocator::underMemoryPressure()
<agomes>  {
<agomes>  #ifdef EXECUTABLE_MEMORY_LIMIT
<agomes>      return DemandExecutableAllocator::bytesAllocatedByAllAllocators() > EXECUTABLE_MEMORY_LIMIT / 2;
<agomes>  #else
<agomes> -    return false;
<agomes> +    return isOverlimit();
<agomes>  #endif
<agomes>  }

[Lyon Chen]

(In reply to comment #3)
> (In reply to comment #2)

Maybe a new cross-platform header file, like WTF/wtf/memorystatus.h, and in this file we define a new function, like isMemoryLow(), but with different implementation for different platforms, and has a default implementation for platforms don't care.

And in ExecutableAllocator::underMemoryPressure(), call this function to decide whether the memory is under pressure or not.

Will create a patch for this.

[Lyon Chen]

Comment on attachment 139198 [details]
Patch for bug 85063

Will create a new patch based on comment 4.

[Lyon Chen]

Created attachment 139216 [details]
Updated patch for 85063 with new file SystemMemoryStatus.h

[Yong Li]

Comment on attachment 139216 [details]
Updated patch for 85063 with new file SystemMemoryStatus.h

View in context: https://bugs.webkit.org/attachment.cgi?id=139216&action=review

Overall it is a much better solution.

> Source/JavaScriptCore/ChangeLog:3
> +        [BlackBerry] Add low memory check in ExecutableAllocator::underMemoryPressure() for BlackBerry platform

This line is no longer accurate and should be changed?

> Source/WTF/wtf/SystemMemoryStatus.h:34
> +#define isMemoryLow BlackBerry::Platform::isMemoryLow

If we must use macro, I would use #define isMemoryLow() BlackBerry::Platform::isMemoryLow.

But I would rather use inline function in "namespace WTF".

[Lyon Chen]

(In reply to comment #7)
> > Source/JavaScriptCore/ChangeLog:3
> > +        [BlackBerry] Add low memory check in ExecutableAllocator::underMemoryPressure() for BlackBerry platform
> 
> This line is no longer accurate and should be changed?

This is from the bug, should I change the title of this bug?

> But I would rather use inline function in "namespace WTF".

OK, will change to use inline for both BlackBerry and non-BlackBerry. And use WTF namespace.

[Lyon Chen]

Update bug title as now it is more a cross-platform solution.

[Lyon Chen]

Created attachment 139229 [details]
Update patch using WTF namespace.

[Yong Li]

Comment on attachment 139229 [details]
Update patch using WTF namespace.

View in context: https://bugs.webkit.org/attachment.cgi?id=139229&action=review

I'm not sure if we should add this to the underLowMemory() implementation in ExecutableAllocatorFixedVMPool.cpp.

Otherwise the patch looks good to me. I'm not a reviewer though :-P

> Source/WTF/wtf/SystemMemoryStatus.h:33
> +#if PLATFORM(BLACKBERRYBBB)

BLACKBERRYBBB? :) A space after this line would be neat

[Lyon Chen]

Created attachment 139236 [details]
Patch fixed typo in 85063.3.patch

[Lyon Chen]

(In reply to comment #11)
> I'm not sure if we should add this to the underLowMemory() implementation in ExecutableAllocatorFixedVMPool.cpp.

Seems we don't need to check low system memory there, as it is checking against the "reserved" memory, so I guess it is not affected by free system memory anyway.

[Lyon Chen]

Created attachment 139247 [details]
Fixed error caused by adding WTF namespace.

[Antonio Gomes]

Comment on attachment 139247 [details]
Fixed error caused by adding WTF namespace.

View in context: https://bugs.webkit.org/attachment.cgi?id=139247&action=review

> Source/WTF/wtf/SystemMemoryStatus.h:45
> +#if PLATFORM(BLACKBERRY)
> +
> +inline bool isSystemMemoryLow() { return BlackBerry::Platform::isMemoryLow(); }
> +
> +#else
> +
> +inline bool isSystemMemoryLow() { return false; }
> +
> +#endif

lets do like this

inline bool isSystemMemoryLow()
{
#if PLATFORM(BLACKBERRY)
    return BlackBerry::Platform::isMemoryLow();
#else
    return false;
#endif
}

[Lyon Chen]

Created attachment 139452 [details]
Patch based on Antonio's Comment 15

[Benjamin Poulain]

Comment on attachment 139452 [details]
Patch based on Antonio's Comment 15

View in context: https://bugs.webkit.org/attachment.cgi?id=139452&action=review

> Source/WTF/wtf/SystemMemoryStatus.h:41
> +#if PLATFORM(BLACKBERRY)
> +    return BlackBerry::Platform::isMemoryLow();
> +#else

I feel WTF might be a weird place for this...

[Benjamin Poulain]

Comment on attachment 139452 [details]
Patch based on Antonio's Comment 15

View in context: https://bugs.webkit.org/attachment.cgi?id=139452&action=review

> Source/WTF/wtf/SystemMemoryStatus.h:41
> +#if PLATFORM(BLACKBERRY)
> +    return BlackBerry::Platform::isMemoryLow();
> +#else

I feel WTF might be a weird place for this...

[Lyon Chen]

(In reply to comment #18)

> I feel WTF might be a weird place for this...

But isn't it a lot of platform specific stuff in WTF already? Or you mean other thing when you said "weird"?

[Benjamin Poulain]

> > I feel WTF might be a weird place for this...
> 
> But isn't it a lot of platform specific stuff in WTF already? Or you mean other thing when you said "weird"?

WebCore already has some support for memory pressure. Adding this transversal isSystemMemoryLow() returning just a bool is rather weird.

[Lyon Chen]

(In reply to comment #20)
> WebCore already has some support for memory pressure. Adding this transversal isSystemMemoryLow() returning just a bool is rather weird.

Thanks for taking the time to review the patch and answer my question.

Sorry I am not familiar with the low memory handling in WebCore, can they also cause an immediate JavaScriptCore garbage collection?

As for us the problem is when the browser is too busy, the garbage collection might not get a chance to execute. And it seems to us checking low memory in ExecutableAllocator::underMemoryPressure() is a nice place to do that check and force a garbage collection.

[Yong Li]

(In reply to comment #20)
> > > I feel WTF might be a weird place for this...
> > 
> > But isn't it a lot of platform specific stuff in WTF already? Or you mean other thing when you said "weird"?
> 
> WebCore already has some support for memory pressure. Adding this transversal isSystemMemoryLow() returning just a bool is rather weird.

There is a layer conflict JSC cannot call WebCore. WebCore can implement a handler for memory pressure, but I think checking memory status should be done in lower layer.

[Benjamin Poulain]

> As for us the problem is when the browser is too busy, the garbage collection might not get a chance to execute. And it seems to us checking low memory in ExecutableAllocator::underMemoryPressure() is a nice place to do that check and force a garbage collection.

For this sounds like this could simply be cutting corners for one particular case.

For me, ExecutableAllocator::underMemoryPressure()  is not the obvious response to low memory conditions. Dropping giant layers and images should be a first step.

This is why I think the memory pressure should be consistent across the whole WebKit and not implemented differently in each component.

> There is a layer conflict JSC cannot call WebCore. WebCore can implement a handler for memory pressure, but I think checking memory status should be done in lower layer.

JSC does call WebCore all the time, that is called the binding ;)

More seriously, the solution could be to move all memory management to WTF. Or it could be that JSC has a callback to know the memory conditions. I don't know what it the best solution, but I think adding something transversal to WebCore might not be the best solution.

[Filip Pizlo]

Comment on attachment 139452 [details]
Patch based on Antonio's Comment 15

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.

[Lyon Chen]

(In reply to comment #24)
> 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.

Thanks for the info and taking the time to review this, Filip & Ben! And sorry for me having not checked this out more carefully.

[Yong Li]

Even just releasing JIT memory is still, probably not much, helpful.

The memory pressure handler in WebCore is good, but it could be too late in some cases when webkit thread is very busy in executing JS.

We could check the system memory status before allocating a new JS block, and perform a GC if necessary.

Adding a callback is also a solution, better than nothing, from my point of view.

[George Staikos]

(In reply to comment #24)
> (From update of attachment 139452 [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.

[Filip Pizlo]

(In reply to comment #27)
> (In reply to comment #24)
> > (From update of attachment 139452 [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.

[George Staikos]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #24)
> > > (From update of attachment 139452 [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?

[Filip Pizlo]

(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, and 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.

[Yong Li]

(In reply to comment #30)
> (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] [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, and 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.

Thanks a lot! I think we should re-evaluate this task based on your comments.