Bug 92505 – [DRT] Replace JavaScriptThreadingPthreads.cpp with JavaScriptThreading.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Patrick R. Gansterer]

[DRT] Replace JavaScriptThreadingPthreads.cpp with JavaScriptThreading.cpp

[Patrick R. Gansterer]

Created attachment 154942 [details]
Patch

[WebKit Review Bot]

Attachment 154942 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/D..." exit_code: 1
Tools/DumpRenderTree/JavaScriptThreading.cpp:82:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 154942 [details]
Patch

Attachment 154942 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13380200

[Patrick R. Gansterer]

Created attachment 154963 [details]
Patch

[WebKit Review Bot]

Attachment 154963 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/D..." exit_code: 1
Tools/DumpRenderTree/JavaScriptThreading.cpp:82:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 154963 [details]
Patch

Could you use svn mv instead of deleting the old file & adding the new file? That way, your change will show up as a diff.

[Patrick R. Gansterer]

Created attachment 196817 [details]
Patch

[Brent Fulgham]

Comment on attachment 196817 [details]
Patch

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?

[Patrick R. Gansterer]

Comment on attachment 196817 [details]
Patch

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

[Brent Fulgham]

Based on paroga's comments, I think this looks fine.  I wish the EWS had processed this, but we'll just have to watch the builders and make sure nothing goes wrong.

[Brent Fulgham]

Comment on attachment 196817 [details]
Patch

After discussing further, I think this change looks good. r=me.

[WebKit Commit Bot]

Comment on attachment 196817 [details]
Patch

Rejecting attachment 196817 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 196817, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
es).
Hunk #7 succeeded at 961 (offset 4 lines).
Hunk #8 succeeded at 983 (offset 4 lines).
Hunk #9 succeeded at 995 (offset 4 lines).
patching file Tools/DumpRenderTree/pthreads/JavaScriptThreadingPthreads.cpp
rm 'Tools/DumpRenderTree/pthreads/JavaScriptThreadingPthreads.cpp'
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Brent Fulgham']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/431283

[Brent Fulgham]

Bah!  Patrick, can you rebase the patch and submit again?  I'll r+ it before things drift again.  :-)

[Patrick R. Gansterer]

Created attachment 201254 [details]
Rebased Patch (only for EWS)

I'll rebased the patch to check it via the EWS, but I'll land it manually via svn to keep track of the rename in the version history.

[Patrick R. Gansterer]

Created attachment 201256 [details]
Rebased Patch (only for EWS)

[WebKit Commit Bot]

Attachment 201256 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj', u'Tools/DumpRenderTree/JavaScriptThreading.cpp', u'Tools/DumpRenderTree/pthreads/JavaScriptThreadingPthreads.cpp']" exit_code: 1
Tools/DumpRenderTree/JavaScriptThreading.cpp:31:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/DumpRenderTree/JavaScriptThreading.cpp:82:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

Committed r149843: <http://trac.webkit.org/changeset/149843>