Bug 90622 - blockfreeing thread doesn't need to wake up frequently when device is idle
Summary: blockfreeing thread doesn't need to wake up frequently when device is idle
Status: RESOLVED DUPLICATE of bug 98084
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-05 10:49 PDT by Yong Li
Modified: 2012-10-05 08:40 PDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2012-07-05 10:49:24 PDT
void BlockAllocator::blockFreeingThreadMain()
{
    while (!m_blockFreeingThreadShouldQuit) {
        // Generally wait for one second before scavenging free blocks. This
        // may return early, particularly when we're being asked to quit.
        waitForRelativeTime(1.0);
        if (m_blockFreeingThreadShouldQuit)
            break;


Actually a conditional signal is being used. So the 1 second timeout doesn't seem
necessary.

Can we change it to infinite?
Comment 1 Filip Pizlo 2012-07-05 10:59:22 PDT
(In reply to comment #0)
> void BlockAllocator::blockFreeingThreadMain()
> {
>     while (!m_blockFreeingThreadShouldQuit) {
>         // Generally wait for one second before scavenging free blocks. This
>         // may return early, particularly when we're being asked to quit.
>         waitForRelativeTime(1.0);
>         if (m_blockFreeingThreadShouldQuit)
>             break;
> 
> 
> Actually a conditional signal is being used. So the 1 second timeout doesn't seem
> necessary.

Where is this imaginary conditional signal?

The block freeing thread does not get signaled when there are free blocks.

> 
> Can we change it to infinite?

No.
Comment 2 Yong Li 2012-07-05 11:06:26 PDT
(In reply to comment #1)
> (In reply to comment #0)
> > void BlockAllocator::blockFreeingThreadMain()
> > {
> >     while (!m_blockFreeingThreadShouldQuit) {
> >         // Generally wait for one second before scavenging free blocks. This
> >         // may return early, particularly when we're being asked to quit.
> >         waitForRelativeTime(1.0);
> >         if (m_blockFreeingThreadShouldQuit)
> >             break;
> > 
> > 
> > Actually a conditional signal is being used. So the 1 second timeout doesn't seem
> > necessary.
> 
> Where is this imaginary conditional signal?
> 
> The block freeing thread does not get signaled when there are free blocks.
> 

Hm... I thought m_freeBlockCondition was for that purpose. Sorry for that.
Comment 3 Filip Pizlo 2012-07-05 11:14:11 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > void BlockAllocator::blockFreeingThreadMain()
> > > {
> > >     while (!m_blockFreeingThreadShouldQuit) {
> > >         // Generally wait for one second before scavenging free blocks. This
> > >         // may return early, particularly when we're being asked to quit.
> > >         waitForRelativeTime(1.0);
> > >         if (m_blockFreeingThreadShouldQuit)
> > >             break;
> > > 
> > > 
> > > Actually a conditional signal is being used. So the 1 second timeout doesn't seem
> > > necessary.
> > 
> > Where is this imaginary conditional signal?
> > 
> > The block freeing thread does not get signaled when there are free blocks.
> > 
> 
> Hm... I thought m_freeBlockCondition was for that purpose. Sorry for that.

Yeah, it's only to tell the thread to quit, not to wake it up.  The block freeing thread in our GC uses exponential decay.  Once a second it frees half the available blocks.  But there's more: if there was an allocation during the last second then it just waits another second.  So the one second wait is quite necessary for the algorithm to work as intended.  That's not to say that's the best way to free blocks, but it works well, if you consider the requirements: 1) you don't ever want the block freeing thread to free all available blocks all at once because there's always the chance that the main thread will want to allocate blocks in the near future, and there's no way to predict when or if that will happen since it depends on user actions, 2) you want all blocks to be returned to the OS eventually if the browser has gone idle, and 3) you don't want the communication between the main thread and the block freeing thread to involve heavy locks.  The timeout is the best way to achieve all of these goals, since it means that even if the browser goes idle for a few seconds, there will still likely be some small number of free blocks left.  In that way, the GC can adapt to whatever level of activity the user imposes.
Comment 4 Yong Li 2012-07-05 11:45:18 PDT
(In reply to comment #3)
> Yeah, it's only to tell the thread to quit, not to wake it up.  The block freeing thread in our GC uses exponential decay.  Once a second it frees half the available blocks.  But there's more: if there was an allocation during the last second then it just waits another second.  So the one second wait is quite necessary for the algorithm to work as intended.  That's not to say that's the best way to free blocks, but it works well, if you consider the requirements: 1) you don't ever want the block freeing thread to free all available blocks all at once because there's always the chance that the main thread will want to allocate blocks in the near future, and there's no way to predict when or if that will happen since it depends on user actions, 2) you want all blocks to be returned to the OS eventually if the browser has gone idle, and 3) you don't want the communication between the main thread and the block freeing thread to involve heavy locks.  The timeout is the best way to achieve all of these goals, since it means that even if the browser goes idle for a few seconds, there will still likely be some small number of free blocks left.  In that way, the GC can adapt to whatever level of activity the user imposes.

Wow. Thanks for this very helpful explanation. I was thinking that we could increase the timeout a little bit when there is no free block found. But it seems unnecessary for now and will complicate the code.
Comment 5 Yong Li 2012-07-07 09:53:52 PDT
I got a local hack for blackberry port. will try to polish it a bit later.

Reopening so I won't forget this one for years.
Comment 6 Filip Pizlo 2012-07-07 13:39:55 PDT
(In reply to comment #5)
> I got a local hack for blackberry port. will try to polish it a bit later.

It would be better if we didn't have platform-local hacks in JavaScriptCore.

> 
> Reopening so I won't forget this one for years.

Is the problem that you're seeing once-a-second wake-ups in the block-freeing thread when the device is completely idle?

This is an interesting problem that we could solve correctly: if after N wake-ups (i.e. N seconds) the block freeing thread has not seen any allocations, then it could just free all remaining blocks on its list in one go and then go to sleep with infinite timeout while setting a flag to say that the next time the GC runs, it should broadcast on the condition to re-wake it.  Better yet, instead of having it infinitely wait, the thread could just shut itself down, thereby freeing stack space.

There's the question of what N should be.  We have a number of options:

A) It could just be dependent on the number of free blocks.  The number of free blocks may take a long time to reach zero due to exponential decay, but we could say that if the number of free blocks drops below some threshold (say 2 blocks, or some platform-specific constant that describes the power cost of registering a one-second timeout to do an action instead of doing the action immediately) then we immediately free them and either shut the thread down or infinite sleep, whichever is more appropriate.

B) It could be a constant, like 20, meaning 20 seconds.  This would mean that if the browser goes idle for 20 seconds then we know two things: due to exponential decay, we would have freed 99.9999% of blocks; and probably the browser is likely to continue to be idle so we should just shut down the block freeing thread or sleep infinitely, whichever is more appropriate.

Thoughts?
Comment 7 Filip Pizlo 2012-07-07 13:40:43 PDT
Changing title/component to reflect the fact that this problem affects JavaScriptCore more broadly and isn't limited to BlackBerry.
Comment 8 Yong Li 2012-07-09 08:07:05 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > I got a local hack for blackberry port. will try to polish it a bit later.
> 
> It would be better if we didn't have platform-local hacks in JavaScriptCore.
> 

I totally agree. Just didn't want to bother other ports when it is still in progress. Also I'm hacking it with device state notifications, which is a platform-specific thing. We could add a platform-independent interface though.

> Is the problem that you're seeing once-a-second wake-ups in the block-freeing thread when the device is completely idle?
>

Basically yes.
 
> This is an interesting problem that we could solve correctly: if after N wake-ups (i.e. N seconds) the block freeing thread has not seen any allocations, then it could just free all remaining blocks on its list in one go and then go to sleep with infinite timeout while setting a flag to say that the next time the GC runs, it should broadcast on the condition to re-wake it.  Better yet, instead of having it infinitely wait, the thread could just shut itself down, thereby freeing stack space.
> 
> There's the question of what N should be.  We have a number of options:
> 
> A) It could just be dependent on the number of free blocks.  The number of free blocks may take a long time to reach zero due to exponential decay, but we could say that if the number of free blocks drops below some threshold (say 2 blocks, or some platform-specific constant that describes the power cost of registering a one-second timeout to do an action instead of doing the action immediately) then we immediately free them and either shut the thread down or infinite sleep, whichever is more appropriate.
> 
> B) It could be a constant, like 20, meaning 20 seconds.  This would mean that if the browser goes idle for 20 seconds then we know two things: due to exponential decay, we would have freed 99.9999% of blocks; and probably the browser is likely to continue to be idle so we should just shut down the block freeing thread or sleep infinitely, whichever is more appropriate.
> 
> Thoughts?

I prefer A) because we are not sure there is no active JS execution happening. Also, the 1-second wake up isn't bad when device is active. I would like to make it gradually slow down when device is idle, and restore to default behavior as soon as device state changes to active, or a GC happens in idle state.
Comment 9 Yong Li 2012-10-05 08:40:21 PDT
fixed by https://bugs.webkit.org/show_bug.cgi?id=98084

*** This bug has been marked as a duplicate of bug 98084 ***