Bug 68429 - [WIN] Remove dependency on pthread from MachineStackMarker
Summary: [WIN] Remove dependency on pthread from MachineStackMarker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on:
Blocks: 68046 88300
  Show dependency treegraph
 
Reported: 2011-09-20 00:45 PDT by Patrick R. Gansterer
Modified: 2012-08-06 23:33 PDT (History)
15 users (show)

See Also:


Attachments
Patch (6.88 KB, patch)
2011-09-20 00:49 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (6.62 KB, patch)
2011-09-20 01:14 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (6.63 KB, patch)
2011-09-20 01:23 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (8.00 KB, patch)
2011-11-17 05:00 PST, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (8.49 KB, patch)
2011-11-17 05:58 PST, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (8.40 KB, patch)
2012-02-21 11:38 PST, Patrick R. Gansterer
ggaren: review-
Details | Formatted Diff | Diff
Patch (9.63 KB, patch)
2012-08-02 02:21 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (10.09 KB, patch)
2012-08-04 01:20 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2011-09-20 00:45:53 PDT
Use WTF::ThreadSpecific instead of pthread_key_t in JSC::MachineThreads
Comment 1 Patrick R. Gansterer 2011-09-20 00:49:20 PDT
Created attachment 107972 [details]
Patch
Comment 2 Patrick R. Gansterer 2011-09-20 01:14:07 PDT
Created attachment 107973 [details]
Patch
Comment 3 WebKit Review Bot 2011-09-20 01:16:36 PDT
Attachment 107973 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/heap/MachineStackMarker.cpp:152:  Missing space inside { }.  [whitespace/braces] [5]
Source/JavaScriptCore/heap/MachineStackMarker.cpp:153:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/JavaScriptCore/heap/MachineStackMarker.cpp:158:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Patrick R. Gansterer 2011-09-20 01:23:13 PDT
Created attachment 107975 [details]
Patch
Comment 5 Patrick R. Gansterer 2011-11-17 05:00:54 PST
Created attachment 115566 [details]
Patch
Comment 6 WebKit Review Bot 2011-11-17 05:03:39 PST
Attachment 115566 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/ThreadSpecificWin.cpp:39:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/ThreadSpecific.h:122:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/ThreadSpecific.h:123:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/ThreadSpecific.h:124:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/ThreadSpecific.h:125:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Gyuyoung Kim 2011-11-17 05:30:29 PST
Comment on attachment 115566 [details]
Patch

Attachment 115566 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10510175
Comment 8 Gustavo Noronha (kov) 2011-11-17 05:49:48 PST
Comment on attachment 115566 [details]
Patch

Attachment 115566 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10509165
Comment 9 Patrick R. Gansterer 2011-11-17 05:58:20 PST
Created attachment 115572 [details]
Patch
Comment 10 WebKit Review Bot 2011-11-17 06:05:54 PST
Attachment 115572 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/ThreadSpecific.h:51:  "pthread.h" already included at Source/JavaScriptCore/wtf/ThreadSpecific.h:48  [build/include] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Brent Fulgham 2011-12-08 14:13:55 PST
Comment on attachment 115572 [details]
Patch

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

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:148
> +        WTF::ThreadSpecificKeyDelete(m_threadSpecific);

Why do you prefix with WTF here, but not in other cases in this file?
Comment 12 Patrick R. Gansterer 2012-02-21 11:38:10 PST
Created attachment 128011 [details]
Patch
Comment 13 Adam Roben (:aroben) 2012-02-22 14:34:43 PST
Comment on attachment 128011 [details]
Patch

Why can't we use WTF::ThreadSpecific (like the bug title suggests)?
Comment 14 Patrick R. Gansterer 2012-02-22 14:37:19 PST
(In reply to comment #13)
> (From update of attachment 128011 [details])
> Why can't we use WTF::ThreadSpecific (like the bug title suggests)?
"missing" constructor: http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/ThreadSpecific.h?rev=107587#L79
Comment 15 Alexey Proskuryakov 2012-02-22 16:00:32 PST
Comment on attachment 128011 [details]
Patch

I wonder if this use of pthread_key_delete is needed.
Comment 16 Adam Roben (:aroben) 2012-02-23 07:22:01 PST
It would be great not to have to introduce a new threading abstraction. If we can find a way to make WTF::ThreadSpecific work, that would be ideal. It sounds like we could use it if we don't need to support the existing pthread_key_delete behavior. Alexey and the JSC folks are the experts here.
Comment 17 Thomas Fletcher 2012-03-05 12:37:27 PST
Is there any thought on this?  Currently this is one of the WinCE build breakers.  If we can't use the ThreadSpecific class then is the desired direction to use the Win32 API for thread specific key creation?
Comment 18 Mark Salisbury 2012-03-06 12:15:04 PST
I'm interested in this too.  I have pthreads compiling for WinCE (not tested yet), but it would be nice to have the flexibility to not use pthreads on WinCE since the project is not being maintained.
Comment 19 Simon Hausmann 2012-03-07 13:59:23 PST
*raises hand to show interest*

Peter, any thoughts on the use of ThreadSpecific based on Adam's comments?
Comment 20 Patrick R. Gansterer 2012-03-08 00:37:10 PST
(In reply to comment #19)
> *raises hand to show interest*
> 
> Peter, any thoughts on the use of ThreadSpecific based on Adam's comments?

Since I do not see any Peter in the CC list, I think that you mean PATRICK.

Please don't get me wrong, but here some points:
*) It is nice to get a "i want this too" every day, but since we are an open source project everybody can post a patch.
*) It is hard do post a "correct" patch, when nobody says what's the "correct" way.
*) The original authors of the pthread stuff i want to remove do not show much participation here. (first patch is from 2011-09)
*) It's windows only.
*) pthread works _somehow_ on windows so no real need in the moment. That it implies additional dependencies for other ports like Qt on windows too isn't a problem for the Apple folks.
*) Like everybody else who works on WebKit in his free time, I prefer changes which get into trunk and make (more) fun to work on.

If someone tells me the way to go, I'm willing to write a patch and post it for review here.
*) Extending the current ThreadSpecific with a Destructor makes the current code even more tricky (unreadable) or adds additional Maps and Sets, which make threads more constable.
*) IMHO MachineStackMarker needs some (bigger) changes to fit into the ThreadSpecific concept, since the current code does not use the pthread_key_t as a thread specific storage. It's used to detect when a thread ends.

Once again: Please don't get me wrong.
Comment 21 Thomas Fletcher 2012-03-08 07:15:45 PST
Well if the goal is to simply remove the blatant pthread dependency in MachineStackMarker then I am happy to provide a patch that addresses that issue so that you can continue to build for Windows platforms without the requirement of a pthread cover library on top of the native threading API.

My comment was less of a "hand up +1" than a question about how can we get this issue solved and moving forward ... and how can I help with that.
Comment 22 Patrick R. Gansterer 2012-03-08 07:45:27 PST
One again: The last comment wasn't agains anybody! (more a general "problem" with this bug)

I had a discussion with Simon this morning about this in IRC and he agreed, that there is no easy/simple and clean solution. If we want a clean solution (and at WebKit we usually go this way) we need to investigate more on this issue. Of course is it possible to change only MachineStackMarker, but we should investigate a clean and generic solution, since we'll have similar requirements in FastMalloc too, if we want to get rid complete pthread stuff on windows.

(In reply to comment #21)
> ... and how can I help with that.
Look into the code and post an idea how to solve this here. I'd suggest that we agree on a solution first (what code do we ant to change/extend) so we avoid any (hard and useless) patch creation. But feel free to do it like you want. :-)

Maybe Simon has an idea of a clean solution in the meantime?
Comment 23 Don Olmstead 2012-03-29 19:08:57 PDT
(In reply to comment #22)
> One again: The last comment wasn't agains anybody! (more a general "problem" with this bug)
> 
> I had a discussion with Simon this morning about this in IRC and he agreed, that there is no easy/simple and clean solution. If we want a clean solution (and at WebKit we usually go this way) we need to investigate more on this issue. Of course is it possible to change only MachineStackMarker, but we should investigate a clean and generic solution, since we'll have similar requirements in FastMalloc too, if we want to get rid complete pthread stuff on windows.
> 
> (In reply to comment #21)
> > ... and how can I help with that.
> Look into the code and post an idea how to solve this here. I'd suggest that we agree on a solution first (what code do we ant to change/extend) so we avoid any (hard and useless) patch creation. But feel free to do it like you want. :-)
> 
> Maybe Simon has an idea of a clean solution in the meantime?

So I've hit this issue as well and wanted to make a potentially ill-informed comment.

So the rub with ThreadSpecific boils down to the destructor not being implemented for a very good reason, undefined behavior. But something like a pthreads implementation of ThreadSpecific would end up calling pthread_key_delete as MachineThreads does in its destructor. The fact that pthread_key_delete is called this way makes it seem like this is okay here because of some knowledge on the lifetime of the pthread_key.

So why not create something like ThreadSpecific<T> but with a destructor. So like a std::unique_ptr we know that only one thing owns it and we don't need to reference count it or do anything funky when determining when to delete it.

Forgive me if I'm missing some larger issue as I'm a relative newbie with this codebase. I'm currently blocked on this issue so I figured I'd chime in.

Cheers,
Don
Comment 24 Joel Dillon 2012-05-02 01:21:18 PDT
As an alternate suggestion, perhaps we could consider using

http://locklessinc.com/articles/pthreads_on_windows/

for Windows? It's implemented entirely in a header file and is BSD licensed, meaning

a) it works for both 32 and 64 bit platforms without problems (whereas there is no precompiled 64 bit version of pthreads-win32, as the name suggests).

and

b) it can be shipped with WebKit as source instead of being an external dependency.
Comment 25 Patrick R. Gansterer 2012-05-02 03:10:57 PDT
(In reply to comment #24)
> As an alternate suggestion, perhaps we could consider using
> 
> http://locklessinc.com/articles/pthreads_on_windows/
> 
> for Windows? It's implemented entirely in a header file and is BSD licensed, meaning

Does it work? IMHO it can't: Threads are created vie CreateThread(), so there is no place where the pthread code for "on thread exit" can be executed. When used as DLL there are some tricks for this, since DLLs get informed about such events and can handle them.
Comment 26 Adam Roben (:aroben) 2012-05-02 05:54:07 PDT
(In reply to comment #25)
> (In reply to comment #24)
> > As an alternate suggestion, perhaps we could consider using
> > 
> > http://locklessinc.com/articles/pthreads_on_windows/
> > 
> > for Windows? It's implemented entirely in a header file and is BSD licensed, meaning
> 
> Does it work? IMHO it can't: Threads are created vie CreateThread(), so there is no place where the pthread code for "on thread exit" can be executed. When used as DLL there are some tricks for this, since DLLs get informed about such events and can handle them.

Since WebKit itself is a DLL, it could make a callback into the pthread implementation whenever a thread is created/destroyed.
Comment 27 Patrick R. Gansterer 2012-05-02 06:27:49 PDT
(In reply to comment #26)
> Since WebKit itself is a DLL, it could make a callback into the pthread implementation whenever a thread is created/destroyed.

But then we need additonal ugly DLL hooks in the WebKit DLL and it won't work when sb does static builds. IMHO it doesn't make much sense to implement any "workarounds" like DLL hooks when there is an easy way to put it into the "normal" codebase.
Comment 28 Simon Hausmann 2012-07-13 04:08:52 PDT
The more I look into this the more I like Patrick's patch.

MachineStackMarker is using two key aspects of the underlying TLS platform API:

    1) Get notified when the thread exits
    2) Store just a pointer in the TLS (no ownership assumed, no deletion required)

When the thread that owns MachineStackMarker exits, then it will just destroy the TLS key to "unsubscribe" from any thread-termination notification. The destruction does not require any data deletion, because of (2).


The WTF::ThreadSpecific interface is designed to store and own a piece of data - it allocates the template parameter T implicitly per thread and it also destroy the per-thread instance.

Aspect (1) of MachineStackMarker could theoretically be mapped to the destructor of a wrapping type in MachineStackMarker, but there is no mechanism in ThreadSpecific to "unsubscribe" from termination deletion.

It would be possible to just solve this with another indirection inside MachineStackMarker, but it comes at the price of the ThreadSpecific remaining alive until all the threads have terminated that were ever "adopted". So in a scenario with two long living threads, one creating and deleting MachineStackMarker instances and the other thread ending up using the JSC instance from time to time, we end up with slowly eating up TLS keys.

One solution I see is to introduce a pointer specific specialization of ThreadSpecific (T*) that would _just_ store a pointer and would allow for a callback when the thread terminates. But that would be a C++/template interface to what Patrick already does with a few helper functions in his patch. And it would make the ThreadSpecific API slightly inconsistent depending on the specialization.

Alexey, Adam: Would you be willing to reconsider Patrick's patch?
Comment 29 Geoffrey Garen 2012-08-01 12:27:52 PDT
Comment on attachment 128011 [details]
Patch

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

> Source/JavaScriptCore/wtf/ThreadSpecific.h:170
> +class ThreadSpecificKeyValue;

"KeyValue" is a very confusing name phrase. Our typical style is to use the word "platform": "PlatformThreadSpecificKey".

> Source/JavaScriptCore/wtf/ThreadSpecific.h:176
> +void ThreadSpecificKeyCreate(ThreadSpecificKey*, void (*)(void *));
> +void ThreadSpecificKeyDelete(ThreadSpecificKey);
> +void ThreadSpecificSet(ThreadSpecificKey, void*);
> +void* ThreadSpecificGet(ThreadSpecificKey);

Capitalization here doesn't match WebKit style.

> Source/JavaScriptCore/wtf/ThreadSpecificWin.cpp:60
> +        while (*next != this) {
> +            ASSERT(*next);
> +            next = &(*next)->m_next;
> +        }
> +        *next = (*next)->m_next;

Linear search is so 1960s. Please use a doubly linked list with O(1) removal time.

> Source/JavaScriptCore/wtf/ThreadSpecificWin.cpp:84
> +    static ThreadSpecificKeyValue* m_first;

WebKit style is to use the "s_" prefix for static data. "m_" means "member" data. Please don't call static data member data.
Comment 30 Patrick R. Gansterer 2012-08-02 02:21:13 PDT
Created attachment 156016 [details]
Patch
Comment 31 Geoffrey Garen 2012-08-02 12:10:00 PDT
Comment on attachment 156016 [details]
Patch

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

> Source/WTF/wtf/ThreadSpecificWin.cpp:128
> +        key->callDestructor();
> +        key = key->next();

Do we expect the destructor to delete the PlatformThreadSpecificKey by calling threadSpecificKeyDelete()? If so, we need to fetch next() before calling the destructor, to avoid dereferencing deleted memory. If not, who do we expect to call threadSpecificKeyDelete()?
Comment 32 Patrick R. Gansterer 2012-08-02 16:04:33 PDT
Comment on attachment 156016 [details]
Patch

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

>> Source/WTF/wtf/ThreadSpecificWin.cpp:128
>> +        key = key->next();
> 
> Do we expect the destructor to delete the PlatformThreadSpecificKey by calling threadSpecificKeyDelete()? If so, we need to fetch next() before calling the destructor, to avoid dereferencing deleted memory. If not, who do we expect to call threadSpecificKeyDelete()?

PlatformThreadSpecificKey will be created via threadSpecificKeyCreate and deleted via threadSpecificKeyDelete. The caller of threadSpecificKeyCreate is responsible to free it via threadSpecificKeyDelete. This is the same behavior as with pthread_key_create and pthread_key_delete. callDestructor() will call the the destructor function provided by threadSpecificKeyCreate, which is stored in m_destructor. The responsibility for freeing objects should be the same as in the pthread implementation. The only difference is, that the thread specific value won't be set to NULL and there is no "PTHREAD_DESTRUCTOR_ITERATIONS".
To be honest: I'm not 100% happy with this API, but I want to get rid of the (silently introduced) pthread dependency. I'm open to change it follow up patches (if there are any good ideas), but I (and other) want this landed, because it's a blocker since 2011(!). The main idea behind this patch is, that it only adds an abstraction to the existing pthread functions.
Comment 33 Geoffrey Garen 2012-08-03 08:26:53 PDT
> The main idea behind this patch is, that it only adds an abstraction to the existing pthread functions.

I don't think you understood my comment. I'm not contesting the goal of this patch. I'm saying the implementation seems to have a "use after free" bug in it.
Comment 34 Patrick R. Gansterer 2012-08-03 08:46:09 PDT
(In reply to comment #33)
> > The main idea behind this patch is, that it only adds an abstraction to the existing pthread functions.
> 
> I don't think you understood my comment. I'm not contesting the goal of this patch. I'm saying the implementation seems to have a "use after free" bug in it.

key->callDestructor() does not free key. key stays valid until threadSpecificKeyDelete() gets called.
Comment 35 Geoffrey Garen 2012-08-03 14:34:24 PDT
> key->callDestructor() does not free key. key stays valid until threadSpecificKeyDelete() gets called.

What happens if a destructor calls threadSpecificKeyDelete()?

From the pthread_key_delete man page:

"The pthread_key_delete() function is callable from within destructor functions."
Comment 36 Patrick R. Gansterer 2012-08-04 01:20:49 PDT
Created attachment 156527 [details]
Patch
Comment 37 Geoffrey Garen 2012-08-06 16:30:19 PDT
Comment on attachment 156527 [details]
Patch

r=me
Comment 38 WebKit Review Bot 2012-08-06 17:29:30 PDT
Comment on attachment 156527 [details]
Patch

Clearing flags on attachment: 156527

Committed r124823: <http://trac.webkit.org/changeset/124823>
Comment 39 WebKit Review Bot 2012-08-06 17:29:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Simon Hausmann 2012-08-06 23:33:32 PDT
Thank you Geoffrey and Patrick!