Summary: | Simplify the WTF thread creation mechanism | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||
Status: | ASSIGNED --- | ||||||||
Severity: | Normal | CC: | andersca, benjamin, cmarcelo, commit-queue | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Mark Lam
2015-07-21 13:28:29 PDT
Created attachment 257201 [details]
the fix.
Uploading to test on the EWS bots, while I'm running perf and regression tests locally.
Created attachment 257207 [details]
patch 2: added missing #include, and #if out unused function in NDEBUG build.
All tests (jsc, api, and layout tests) show no regressions. Benchmarks (jsc, jetstream, speedstream) shows that this patch is perf neutral. Comment on attachment 257207 [details] patch 2: added missing #include, and #if out unused function in NDEBUG build. View in context: https://bugs.webkit.org/attachment.cgi?id=257207&action=review > Source/WTF/ChangeLog:51 > + This does not work when we want to use std::thread::id as the implementation of ThreadIdentifier later. > + That requires the ThreadIdentifier to be established on the created thread instead because there is no > + way std::thread::get_id() that can be invoked on a thread that is not the current one. I'm not certain what you mean by this, but I think you've missed something. See http://en.cppreference.com/w/cpp/thread/thread/get_id. Comment on attachment 257207 [details] patch 2: added missing #include, and #if out unused function in NDEBUG build. View in context: https://bugs.webkit.org/attachment.cgi?id=257207&action=review I don't think this patch is a simplification. Perhaps a new patch, inspired by some of the ideas in this patch, would be a simplification. > Source/WTF/wtf/ThreadingPthreads.cpp:180 > + volatile ThreadIdentifier id { 0 }; "volatile" is wrong here. > Source/WTF/wtf/ThreadingPthreads.cpp:188 > + // The contents of the ThreadData is only guaranteed to be valid before we signal the creating > + // thread below. The signaling tells the creating thread that we're done initializing and implies > + // that it may dispose of the ThreadData. Hence, we should grab the info we need out of it > + // before then. This is a strange dance. Why not have the creating thread transfer owner of ThreadData to the created thread? That's a much easier model to work with. > Source/WTF/wtf/ThreadingPthreads.cpp:225 > + while (!data.id) > + data.conditionVariable.wait(lock); Why do we want the creating thread to wait for the created thread to come into existence? That seems potentially slow, and a regression relative to what the old code did. > Source/WTF/wtf/ThreadingPthreads.cpp:226 > + ASSERT(data.id); I don't like this ASSERT, since it duplicates the while loop just above it. > Source/WTF/wtf/ThreadingWin.cpp:212 > + volatile ThreadIdentifier id { 0 }; "volatile" is wrong here. > Source/WTF/wtf/ThreadingWin.cpp:231 > +#if !defined(NDEBUG) > + data->id = currentThread(); > +#endif > + data->done = true; Why not use id to indicate done-ness, as we do in pthreads? > Source/WTF/wtf/ThreadingWin.cpp:264 > + while (!data.done) > + data.conditionVariable.wait(lock); Why do we want the creating thread to wait for the created thread to come into existence? That seems potentially slow, and a regression relative to what the old code did. |