[Webkit-unassigned] [Bug 97539] Broken and incorrect code in FastMalloc.cpp

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Mark Toller <mark.toller at samsung.com>  2012-09-27 07:01:34 PST ---
(In reply to comment #10)
> > 1) The comment "m_scavengeMutex should be held before accessing m_scavengeThreadActive." is misleading, it should be stating "the caller [of signalScavenger()] should ensure that m_scavengeMutex is locked prior to calling this method."
> 
> Yes, I like your variant more. I'd have said "taken" and not "locked", but either works for me.
> 
> > I'm not sure if your comment is based on a misunderstanding of the pthread_cond_wait(&m_scavengeCondition, &m_scavengeMutex); code - this atomically waits on the condvar and unlocks the mutex.
> 
> I wrote this code a few years ago, and that the logic was supposed to match what's in the version that is actually used:
> 
> ALWAYS_INLINE void TCMalloc_PageHeap::signalScavenger()
> {
>     ASSERT(pageheap_lock.IsHeld());
> ...
> 
> Of course, being an unused branch, it was not tested, and didn't even compile. It's quite possible that a major cleanup of the algorithm is needed there.

Ok, I see. Actually, this branch isn't unused as such - it is used by WebkitGtk (GtkLauncher), but as I said earlier, without modifying the build/code, you can't compile using TCMalloc AND debug (so the ASSERT was never compiled in).

So, in the GTK/linux variant (no DISPATCH_H and !Windows) a condvar is used to make the scavenger thread sleep. As the condvar requires a mutex, it would seem logical to use the mutex to protect m_scavengeThreadActive - however, the mutex exists only for this convar / thread, and isn't locked (like the pageheap_lock) on entry to 'signalScavenger'. I doubt the overhead of locking/unlocking the mutex on every delete call is desirable?

So, how about using the pageheap_lock spinlock to protect m_scavengeThreadActive? The mutex becomes purely a side-effect of using the condvar to make the thread sleep, and brings the code more in line with the other branches...

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