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

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request 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 Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=196817&action=review


Thanks for working on this -- I'm so excited to almost see PThreads gone on
Windows!

Overall I think this looks good, but I have some concerns that these
modifications will change behavior.  Of course, perhaps the changes will be for
the better -- but I can't tell based on this bug.

Could you please add more documentation in the bug as to why you changed the
flow of the program, and what (if any) changes in behavior should be expected?

I don't see EWS results for this change, so I can't assess whether this is safe
or not.

For these reasons, I'm marking r- until we clear up some of these questions.

> 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?

> 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?

> 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?

> Tools/DumpRenderTree/JavaScriptThreading.cpp:-109
> -    javaScriptThreads()->remove(pthread_self());

Oh, I see.  The remove was happening here.

Still, why create the thread in a non-detached state?  I think this could have
an impact on the way the tests behave.

> 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.

> Tools/DumpRenderTree/JavaScriptThreading.cpp:135
> +	   javaScriptThreadsShouldTerminate = true;

Huh -- I see that we *were* locking to update the boolean here.  Maybe this
wasn't necessary?