[webkit-reviews] review denied: [Bug 22567] HashTable needs to be made usable from multiple threads. : [Attachment 25620] Added copyright to HashTable.h

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied David Levin
<levin at chromium.org>'s request for review:
Bug 22567: HashTable needs to be made usable from multiple threads.
https://bugs.webkit.org/show_bug.cgi?id=22567

Attachment 25620: Added copyright to HashTable.h
https://bugs.webkit.org/attachment.cgi?id=25620&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
 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?