Bug 65382 – The JSC garbage collector returns memory to the operating system too eagerly

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

The JSC garbage collector will, in full-sweep mode, return every free block to the operating system.  Subsequent memory requests will have a high probability of having to go back to the OS to re-request that same memory.  This is not efficient.  The GC should instead hold on to free memory for a little while, in case that memory will immediately be re-requested by the application code.  If it does not get requested by application code after a few seconds, it should be returned to the OS.

[Filip Pizlo]

Created attachment 102383 [details]
the patch

This is a work-in-progress patch.  It still needs a bit more testing.

[WebKit Review Bot]

Attachment 102383 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/heap/Heap.cpp:316:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/heap/Heap.cpp:332:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/heap/Heap.cpp:338:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/heap/Heap.cpp:339:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 102385 [details]
the patch (fix style)

Layout tests are now passing. This patch appears to be a 3-6% speed-up on SunSpider, depending on hardware, and about a 9-11% speed-up overall on V8, depending on how you run it (the V8 harness versus my harness).

[Filip Pizlo]

Created attachment 102406 [details]
the patch (fix annoying, though benign, bug)

This patch improves on the previous one by fixing a nasty, but benign, bug.  When returning a block to the system, the scavenger thread calls MarkedBlock::destroy().  This method first calls MarkedBlock::reset() and then frees the memory.  But MarkedBlock::reset() would have already been called from the main thread during the GC sweep.  Calling it again is risky, since reset() invokes destructors on cells in the block.  It just so happens that in all cases where blocks are placed on the free list, the block would have been swept and and the cells within it would be blank JSCells - so invoking the destructor twice would be asymptomatic.  But, fixing this issue makes the logic more consistent and decreases the likelihood of problems in the future.

[WebKit Review Bot]

Comment on attachment 102406 [details]
the patch (fix annoying, though benign, bug)

Rejecting attachment 102406 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
ceeded at 678 (offset -6 lines).
1 out of 5 hunks FAILED -- saving rejects to file Source/JavaScriptCore/heap/Heap.cpp.rej
patching file Source/JavaScriptCore/heap/Heap.h
patching file Source/JavaScriptCore/heap/MarkedBlock.cpp
Hunk #1 succeeded at 42 (offset -1 lines).
Hunk #2 succeeded at 50 (offset -1 lines).
patching file Source/JavaScriptCore/heap/MarkedBlock.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Oliver Hunt', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9286011

[Oliver Hunt]

Looks like this may need to be updated to ToT

[Filip Pizlo]

Created attachment 102448 [details]
the patch (fix conflicts)

[Oliver Hunt]

Comment on attachment 102448 [details]
the patch (fix conflicts)

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

Not r+/r- because i don't understand why this doesn't lead to the main thread blocking on the release-thread

> Source/JavaScriptCore/heap/Heap.cpp:319
> +    MutexLocker locker(m_freeBlockLock);

Do we really want to wait while holding the freeBlock lock?  It looks like that would lead to allocateBlock blocking for up to a second on this secondary thread?

> Source/JavaScriptCore/heap/Heap.h:39
> +#if ENABLE(SINGLE_THREADED)
> +#define HEAP_FREE_BLOCKS_EAGERLY 1
> +#else
> +#define HEAP_FREE_BLOCKS_EAGERLY 0
> +#endif

I think this would read better in general if this became WTF_ENABLE_EAGER_BLOCK_FREEING, and then the call sites work can be guarded with ENABLE(EAGER_BLOCK_FREEING) which is our more typical approach.

[Filip Pizlo]

(In reply to comment #8)
> (From update of attachment 102448 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102448&action=review
> 
> Not r+/r- because i don't understand why this doesn't lead to the main thread blocking on the release-thread
> 
> > Source/JavaScriptCore/heap/Heap.cpp:319
> > +    MutexLocker locker(m_freeBlockLock);
> 
> Do we really want to wait while holding the freeBlock lock?  It looks like that would lead to allocateBlock blocking for up to a second on this secondary thread?

ThreadCondition::timedWait(Mutex, double) should atomically release the Mutex and engage the wait.  So allocateBlock() may wait for the handful of microseconds that it takes to acquire the Mutex and commence waiting, but never a full second (well unless the scavenger thread page faults or something, and the page fault is super-expensive, or something).

The reason why I use ThreadCondition::timedWait() is that it makes heap destruction nicely atomic.  The reason why I use one Mutex, instead of multiple Mutexes, is because I like using one lock for everything unless there is a performance argument to be made.  This makes it easier to reason about the code being race-free, at least to me.  The scavenger thread acquiring the allocation mutex for a few microseconds once a second does not lead to a performance problem, as it means that there is only a 1/1,000,000 chance that the allocator would ever attempt to allocate the mutex at a time when it is held by the scavenger.

I'm more worried that if the scavenger starts scavenging and there are a lot of free blocks, and the allocator is allocating blocks, then they'll thrash on each other a bit, I guess.  But I've not seen this happen in benchmarks.  The right fix would be to have the allocator estimate its current allocation rate.  If the scavenger detects that the allocator will deplete free pages in under a second given its current allocation rate, then it probably shouldn't scavenge anything.  I'll save that for a future patch.  And I'll probably only do it if we have evidence that it's necessary.

> 
> > Source/JavaScriptCore/heap/Heap.h:39
> > +#if ENABLE(SINGLE_THREADED)
> > +#define HEAP_FREE_BLOCKS_EAGERLY 1
> > +#else
> > +#define HEAP_FREE_BLOCKS_EAGERLY 0
> > +#endif
> 
> I think this would read better in general if this became WTF_ENABLE_EAGER_BLOCK_FREEING, and then the call sites work can be guarded with ENABLE(EAGER_BLOCK_FREEING) which is our more typical approach.

I presume you would recommend putting this in wtf/Platform?  Or is it better to keep it in Heap.h?

[Oliver Hunt]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 102448 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=102448&action=review
> > 
> > Not r+/r- because i don't understand why this doesn't lead to the main thread blocking on the release-thread
> > 
> > > Source/JavaScriptCore/heap/Heap.cpp:319
> > > +    MutexLocker locker(m_freeBlockLock);
> > 
> > Do we really want to wait while holding the freeBlock lock?  It looks like that would lead to allocateBlock blocking for up to a second on this secondary thread?
> 
> ThreadCondition::timedWait(Mutex, double) should atomically release the Mutex and engage the wait.  So allocateBlock() may wait for the handful of microseconds that it takes to acquire the Mutex and commence waiting, but never a full second (well unless the scavenger thread page faults or something, and the page fault is super-expensive, or something).

righto

> > 
> > > Source/JavaScriptCore/heap/Heap.h:39
> > > +#if ENABLE(SINGLE_THREADED)
> > > +#define HEAP_FREE_BLOCKS_EAGERLY 1
> > > +#else
> > > +#define HEAP_FREE_BLOCKS_EAGERLY 0
> > > +#endif
> > 
> > I think this would read better in general if this became WTF_ENABLE_EAGER_BLOCK_FREEING, and then the call sites work can be guarded with ENABLE(EAGER_BLOCK_FREEING) which is our more typical approach.
> 
> I presume you would recommend putting this in wtf/Platform?  Or is it better to keep it in Heap.h?

I initially thought putting it in Platform.h would be better, but i'm not sure we really need to have this setting trigger a world rebuild, so i decided not to suggest it :D

[Filip Pizlo]

Created attachment 102453 [details]
the patch (use the ENABLE() thingy)

[Filip Pizlo]

Created attachment 102454 [details]
the patch (fix change log)

Realized that I forgot to change the ChangeLog to say that I changed Platform.h

[WebKit Review Bot]

Comment on attachment 102454 [details]
the patch (fix change log)

Clearing flags on attachment: 102454

Committed r92084: <http://trac.webkit.org/changeset/92084>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

Have you considered using libdispatch or similar to avoid having a dedicated thread for this periodic work?

[Filip Pizlo]

(In reply to comment #15)
> Have you considered using libdispatch or similar to avoid having a dedicated thread for this periodic work?

This is a good point!  dispatch_after() would be great here.  One thing that I'm not sure about is how easy it is to cancel a dispatch_after(); the Heap destructor will want to ensure that it can immediately prevent the scavenger from running again, unless it is currently running, in which case it should wait for it to finish.  This is very easy to do with threading primitives.

Can we rely on libdispatch being available on all of the platforms that WebKit runs on?  If it is, then awesome; if not, then I'm dubious about the risk/reward: the risk is that we have two different implementations of the logic for starting, stopping, and scheduling scavenging.  The benefit is that we have one fewer thread.  It's not clear to me that the benefit outweighs the risk.

[Adam Roben (:aroben)]

(In reply to comment #16)
> (In reply to comment #15)
> > Have you considered using libdispatch or similar to avoid having a dedicated thread for this periodic work?
> 
> This is a good point!  dispatch_after() would be great here.  One thing that I'm not sure about is how easy it is to cancel a dispatch_after(); the Heap destructor will want to ensure that it can immediately prevent the scavenger from running again, unless it is currently running, in which case it should wait for it to finish.  This is very easy to do with threading primitives.

You could use dispatch_suspend to suspend the queue on which the scavenger will run, and use a standard mutex to block until the scavenger finishes running.

> Can we rely on libdispatch being available on all of the platforms that WebKit runs on?

We cannot.

> If it is, then awesome; if not, then I'm dubious about the risk/reward: the risk is that we have two different implementations of the logic for starting, stopping, and scheduling scavenging.  The benefit is that we have one fewer thread.  It's not clear to me that the benefit outweighs the risk.

Ideally we'd have an abstraction that provides the same functionality as libdispatch but with a cross-platform interface. WebKit2's WorkQueue abstraction might be a good starting point.

[Yong Li]

Comment on attachment 102454 [details]
the patch (fix change log)

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

> Source/JavaScriptCore/heap/Heap.cpp:334
> +        // Generally wait for one second before scavenging free blocks. This
> +        // may return early, particularly when we're being asked to quit.
> +        waitForRelativeTime(1.0);

Filip, is there any special reason for this one second timeout? Can it be 10 seconds for example? Or infinite?

[Filip Pizlo]

(In reply to comment #18)
> (From update of attachment 102454 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102454&action=review
> 
> > Source/JavaScriptCore/heap/Heap.cpp:334
> > +        // Generally wait for one second before scavenging free blocks. This
> > +        // may return early, particularly when we're being asked to quit.
> > +        waitForRelativeTime(1.0);
> 
> Filip, is there any special reason for this one second timeout? Can it be 10 seconds for example? Or infinite?

Not really.  It's the answer to the question: "how long after I stop doing stuff in my browser do I expect my browser to return memory to the OS?"

Shortening this timeout will slow things down because the block freeing thread would then contend on the allocation mutex more frequently.

Lengthening the timeout may cause other applications to experience longer periods of memory starvation while the browser hoards memory.

One second feels like the right compromise.