Bug 51855 - Extract ThreadFunctionInvocation into separate file and share between Apple Windows and Android
Summary: Extract ThreadFunctionInvocation into separate file and share between Apple W...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-03 17:20 PST by Daniel Bates
Modified: 2011-01-04 10:56 PST (History)
4 users (show)

See Also:


Attachments
Patch (8.00 KB, patch)
2011-01-03 17:40 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (7.12 KB, patch)
2011-01-03 18:11 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (7.14 KB, patch)
2011-01-03 18:15 PST, Daniel Bates
aroben: review-
Details | Formatted Diff | Diff
Patch (7.76 KB, patch)
2011-01-04 10:37 PST, Daniel Bates
aroben: review+
aroben: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2011-01-03 17:20:45 PST
Both the Apple Windows and Android ports implement a similar adapter structure, called ThreadFunctionInvocation and ThreadData respectively, as part of their thread creation process. We should share such an adapter structure and remove duplicate code.
Comment 1 Daniel Bates 2011-01-03 17:40:16 PST
Created attachment 77855 [details]
Patch

This patch also modifies createThreadInternal() in wtf/ThreadingWin.cpp so that we deallocate the instantiated ThreadFunctionInvocation if the call to CreateThread() fails.
Comment 2 Daniel Bates 2011-01-03 18:11:39 PST
Created attachment 77860 [details]
Patch

I thought it would be better to extract the fix for the memory leak into a separate bug, bug #51860.
Comment 3 Daniel Bates 2011-01-03 18:15:46 PST
Created attachment 77861 [details]
Patch

Substitute typedef for ThreadFunction for #include "Threading.h" in ThreadFunctionInvocation.h
Comment 4 Adam Roben (:aroben) 2011-01-04 06:26:50 PST
Comment on attachment 77861 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77861&action=review

> Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:-155
> -    ThreadData* data = static_cast<ThreadData*>(arg);
> +    ThreadFunctionInvocation invocation = *static_cast<ThreadFunctionInvocation*>(arg);
> +    delete static_cast<ThreadFunctionInvocation*>(arg);
> +
>      JavaVM* vm = JSC::Bindings::getJavaVM();
>      JNIEnv* env;
>      void* ret = 0;
>      if (vm->AttachCurrentThread(&env, 0) == JNI_OK) {
> -        ret = data->entryPoint(data->arg);
> +        ret = invocation.function(invocation.data);
>          vm->DetachCurrentThread();
>      }
> -    delete data;

This is a little bizarre. How about just adopting arg into an OwnPtr<ThreadFunctionInvocation> and using that to invoke the function?

> Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:162
> +    ThreadFunctionInvocation* invocation = new ThreadFunctionInvocation(entryPoint, data); // Deallocated in runThreadWithRegistration() if a thread is created.
> +
> +    if (pthread_create(&threadHandle, 0, runThreadWithRegistration, invocation)) {

It would be nicer to use adoptPtr/leakPtr here.
Comment 5 Daniel Bates 2011-01-04 10:03:12 PST
(In reply to comment #4)
> (From update of attachment 77861 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77861&action=review
> 
> > Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:-155
> > -    ThreadData* data = static_cast<ThreadData*>(arg);
> > +    ThreadFunctionInvocation invocation = *static_cast<ThreadFunctionInvocation*>(arg);
> > +    delete static_cast<ThreadFunctionInvocation*>(arg);
> > +
> >      JavaVM* vm = JSC::Bindings::getJavaVM();
> >      JNIEnv* env;
> >      void* ret = 0;
> >      if (vm->AttachCurrentThread(&env, 0) == JNI_OK) {
> > -        ret = data->entryPoint(data->arg);
> > +        ret = invocation.function(invocation.data);
> >          vm->DetachCurrentThread();
> >      }
> > -    delete data;
> 
> This is a little bizarre. How about just adopting arg into an OwnPtr<ThreadFunctionInvocation> and using that to invoke the function?

Will change.

> 
> > Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:162
> > +    ThreadFunctionInvocation* invocation = new ThreadFunctionInvocation(entryPoint, data); // Deallocated in runThreadWithRegistration() if a thread is created.
> > +
> > +    if (pthread_create(&threadHandle, 0, runThreadWithRegistration, invocation)) {
> 
> It would be nicer to use adoptPtr/leakPtr here.

Is it safe to use adoptPtr/leakPtr for <https://bugs.webkit.org/show_bug.cgi?id=51855>. From my understanding, pthread_create() does not take ownership of the parameter arg. Assume we use adoptPtr/leakPtr and suppose pthread_create() returns an error (say EAGAIN - system lacked the necessary resources to create another thread). Then we will not delete the instance of ThreadFunctionInvocation because OwnPtr::leakPtr() sets OwnPtr::m_ptr to 0 and hence ~OwnPtr() will call delete(0).
Comment 6 Adam Roben (:aroben) 2011-01-04 10:24:39 PST
Comment on attachment 77861 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77861&action=review

>>> Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:162
>>> +    if (pthread_create(&threadHandle, 0, runThreadWithRegistration, invocation)) {
>> 
>> It would be nicer to use adoptPtr/leakPtr here.
> 
> Is it safe to use adoptPtr/leakPtr for <https://bugs.webkit.org/show_bug.cgi?id=51855>. From my understanding, pthread_create() does not take ownership of the parameter arg. Assume we use adoptPtr/leakPtr and suppose pthread_create() returns an error (say EAGAIN - system lacked the necessary resources to create another thread). Then we will not delete the instance of ThreadFunctionInvocation because OwnPtr::leakPtr() sets OwnPtr::m_ptr to 0 and hence ~OwnPtr() will call delete(0).

You'd have to do something like:

OwnPtr<> invocation = adoptPtr(…);
if (pthread_create(…, invocation.get())
    return 0;
// The thread will take ownership of invocation.
invocation.leakPtr();

Maybe that's not much better than what you have now.
Comment 7 Daniel Bates 2011-01-04 10:37:57 PST
Created attachment 77900 [details]
Patch

Updated patch based on Adam Roben's suggestions.
Comment 8 Adam Roben (:aroben) 2011-01-04 10:46:20 PST
Comment on attachment 77900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77900&action=review

> Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:161
> +    if (pthread_create(&threadHandle, 0, runThreadWithRegistration, invocation)) {

This shouldn't compile. If it does, we should find out why. You should be passing invocation.get().
Comment 9 Daniel Bates 2011-01-04 10:48:08 PST
(In reply to comment #8)
> (From update of attachment 77900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77900&action=review
> 
> > Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:161
> > +    if (pthread_create(&threadHandle, 0, runThreadWithRegistration, invocation)) {
> 
> This shouldn't compile. If it does, we should find out why. You should be passing invocation.get().

Right. Will fix before I land.
Comment 10 Build Bot 2011-01-04 10:52:10 PST
Attachment 77900 [details] did not build on win:
Build output: http://queues.webkit.org/results/7346026
Comment 11 Daniel Bates 2011-01-04 10:56:48 PST
Committed r74975: <http://trac.webkit.org/changeset/74975>