Bug 22567 – HashTable debug-only data needs to be made usable from multiple threads

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Levin]

With workers running on threads, HashTable needs to have it use of statics made safe with respect to threads.  Also, HashTable::contains should be threadsafe.

[David Levin]

Created attachment 25619 [details]
Fix for bug.

[David Levin]

Created attachment 25620 [details]
Added copyright to HashTable.h

[Alexey Proskuryakov]

Comment on attachment 25620 [details]
Added copyright to HashTable.h

 HashTableStats::~HashTableStats()
 {
> +    MutexLocker lock(hashTableStatsMutex());

Locking a mutex in static object destructor can cause deadlocks at shutdown if any thread was killed during process shutdown while holding this lock.

>      printf("\nkhtml::HashTable statistics\n\n");

This is a good opportunity to rename khtml::HashTable to WTF::HashTable.

>  void HashTableStats::recordCollisionAtCount(int count)
>  {
> +    MutexLocker lock(hashTableStatsMutex());

This works correctly, but only because HashTableStats data members that are used in this function are not used anywhere else (except for HashTableStats dtor). It would help if these data members were re-ordered in HashTableStats to form a single group with a comment explaining how they are used.

> +// Thread safety: HashTable is only thread-safe with respect to the lookup/contains methods.
> +// Any other method use (include any iterators) should have locks around them when used from multiple threads.

I do not believe that this is formally true - lookup is certainly not safe if the table is being modified from another thread at the same time.

But also, this description is quite vague, because it is not clear whether it talks about class-level of object-level thread safety. And it is misguiding, because locking mutexes is not the only form of synchronization that can guarantee safety.

I'm not sure how to best describe HashTable threading model. It is pretty common for container objects to have class-level thread safety, and to have constant methods work on constant objects without synchronization though.

> -        static int numAccesses;
> +        static volatile int numAccesses;

Is this change needed?

[David Levin]

Created attachment 25623 [details]
Addressed comments.

Thanks.

I've made the remaining const methods on HashTable thread safe as well.

> > -        static int numAccesses;
> > +        static volatile int numAccesses;
> Is this change needed?

The function definition includes it "atomicIncrement(int volatile*)".  The only place I could find this function used was in RefCountedLeakCounter.cpp and the variable that it uses for atomicIncrement is defined as volatile.

After thinking about it more, I realized that this is like the const modifier in a prototype which indicates that the function can handle a volatile int* (as well as an int*), so I've removed the volatile for all of these variables.

[Alexey Proskuryakov]

Comment on attachment 25623 [details]
Addressed comments.

> +        Guarded with variable access with a mutex.

An extra "with" here.

> +    // Don't lock the mutex here bcause it can cause deadlocks at shutdown 
> +    // if any thread was killed during process shutdown while holding the mutex.

Typo: bcause.

r=me

[Alexey Proskuryakov]

Committed revision 38859.