[webkit-reviews] review requested: [Bug 92505] [DRT] Replace JavaScriptThreadingPthreads.cpp with JavaScriptThreading.cpp : [Attachment 196817] Patch

[bugzilla-daemon at webkit.org]

Patrick R. Gansterer <paroga at paroga.com> has asked  for review:
Bug 92505: [DRT] Replace JavaScriptThreadingPthreads.cpp with
JavaScriptThreading.cpp
https://bugs.webkit.org/show_bug.cgi?id=92505

Attachment 196817: Patch
https://bugs.webkit.org/attachment.cgi?id=196817&action=review

------- Additional Comments from Patrick R. Gansterer <paroga at paroga.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=196817&action=review


>> Tools/DumpRenderTree/JavaScriptThreading.cpp:99
>> +		if (javaScriptThreadsShouldTerminate)
> 
> Why is this now protected by a mutex? Reading a boolean should be an atomic
operation; did you find this was causing you problems?

see other comment

>> Tools/DumpRenderTree/JavaScriptThreading.cpp:104
>> +	    if (rand() % 5)
> 
> Why the change from random() to rand()?  Is this just to be compatible
between Windows and others?

yup, but I can keep random() in a first step if you like

>> Tools/DumpRenderTree/JavaScriptThreading.cpp:107
>> +	    MutexLocker locker(javaScriptThreadsMutex());
> 
> The logic of the following section seems to change the behavior of the
original code.	Previously the flow was:
> 1. Lock mutex.
> 2. Create a new thread
> 3. Place it in the detached state (this could have perhaps been done by using
pthread_attr_setdetachstate to setup flags for the pthread_create call.
> 4. Thread is added to the queue.
> 5. Unlock mutex.
> 
> I like the use of the mutex object to control lifetime, but the logic now
seems to be:
> 1. Lock mutex.
> 2. Get current running thread, and mark it as detached (presumable it was in
a 'join' state before?)
> 3. Remove the current thread from the queue.
> 4. Create a new thread and add it to the thread queue.
> 5. (automatically Unlock mutex).
> 
> Why the change?  Am I misunderstanding your intent with this change?

I merged the implementation with
http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/win/DumpRenderTree.cp
p?rev=149716#L1127, since I find it easier to understand and implement with the
WTF threading API.
It might have a different timing, but should fulfill all requirements, for
which this code was introduced.

>> Tools/DumpRenderTree/JavaScriptThreading.cpp:128
>> +	    javaScriptThreads().add(createThread(&runJavaScriptThread, 0, 0));
> 
> These threads are not in a detached state.  If they terminate early, I don't
think they get cleaned up properly.

But we call waitForThreadCompletion() for _all_ threads now, which should take
care of cleanup.

>> Tools/DumpRenderTree/JavaScriptThreading.cpp:135
>> +	    javaScriptThreadsShouldTerminate = true;
> 
> Huh -- I see that we *were* locking to update the boolean here.  Maybe this
wasn't necessary?

I think so too, but didn't want to change behavior.