[Webkit-unassigned] [Bug 65382] The JSC garbage collector returns memory to the operating system too eagerly

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Filip Pizlo <fpizlo at apple.com>  2011-07-30 19:34:47 PST ---
(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?

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