Bug 61017 - [chromium] Turn on WTF_MULTIPLE_THREADS.
Summary: [chromium] Turn on WTF_MULTIPLE_THREADS.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dmitry Lomov
URL:
Keywords:
Depends on:
Blocks: 61016
  Show dependency treegraph
 
Reported: 2011-05-17 19:08 PDT by David Levin
Modified: 2011-07-27 22:16 PDT (History)
6 users (show)

See Also:


Attachments
Quick test on bots (2.62 KB, patch)
2011-07-22 15:15 PDT, Dmitry Lomov
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Enables WTF_MULTIPLE_THREADS in chromium port (8.09 KB, patch)
2011-07-27 14:44 PDT, Dmitry Lomov
levin: review-
Details | Formatted Diff | Diff
CR feedback (8.09 KB, patch)
2011-07-27 16:56 PDT, Dmitry Lomov
levin: review+
levin: commit-queue-
Details | Formatted Diff | Diff
CR feedback + wtfThreadData (9.02 KB, patch)
2011-07-27 17:13 PDT, Dmitry Lomov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2011-05-17 19:08:49 PDT
We need to turn this on to make wtf safe to use in various threads.

To do this, we likely need to move several items from initializeThreadingOnce to something in wtf (perhaps WTF::initializeThreading()).

We also need to fix up FastMalloc.cpp (perhaps like https://bugs.webkit.org/attachment.cgi?id=92838&action=prettypatch but only for Chromium Windows not Windows in general.)

There are some other helpful comments starting at https://bugs.webkit.org/show_bug.cgi?id=55728#c40.
Comment 1 Dmitry Lomov 2011-07-22 15:15:28 PDT
Created attachment 101776 [details]
Quick test on bots
Comment 2 WebKit Review Bot 2011-07-22 16:17:03 PDT
Comment on attachment 101776 [details]
Quick test on bots

Attachment 101776 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9231232

New failing tests:
animations/combo-transform-rotate+scale.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/network/network-cachedresources-with-same-urls.html
http/tests/inspector/change-iframe-src.html
http/tests/inspector/network/network-disable-cache-memory.html
http/tests/inspector/network/network-iframe-load-and-delete.html
http/tests/inspector/network/network-disable-cache-xhrs.html
http/tests/inspector/network/download.html
http/tests/inspector/network-preflight-options.html
animations/big-rotation.html
http/tests/inspector/network/network-open-load-reopen.html
animations/animation-add-events-in-handler.html
http/tests/inspector/network/network-content-replacement-xhr.html
http/tests/inspector/network/network-close-load-open.html
animations/simultaneous-start-transform.html
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/resource-parameters.html
http/tests/inspector/network/network-clear-after-disabled.html
animations/combo-transform-translate+scale.html
Comment 3 Dmitry Lomov 2011-07-27 14:44:01 PDT
Created attachment 102186 [details]
Enables WTF_MULTIPLE_THREADS in chromium port

No cq? - waiting for chromium trybots
Comment 4 David Levin 2011-07-27 15:02:05 PDT
Comment on attachment 102186 [details]
Enables WTF_MULTIPLE_THREADS in chromium port

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

> Source/JavaScriptCore/runtime/InitializeThreading.cpp:59
>      initializeDates();

Why not remove this too?

(and the String::Empty initialization).

> Source/JavaScriptCore/wtf/FastMalloc.cpp:74
> +// * allocation of a reasonably complicated sfEtruct

whoops

> Source/JavaScriptCore/wtf/FastMalloc.cpp:83
> +#if OS(WINDOWS)

and chromium?

I think this already compiles fine on Windows for Apple's port. (Ditto for the OS(WINDOWS) below).
Comment 5 Dmitry Lomov 2011-07-27 15:16:40 PDT
Comment on attachment 102186 [details]
Enables WTF_MULTIPLE_THREADS in chromium port

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

>> Source/JavaScriptCore/runtime/InitializeThreading.cpp:59
>>      initializeDates();
> 
> Why not remove this too?
> 
> (and the String::Empty initialization).

Done, sorry forgot to do that.

>> Source/JavaScriptCore/wtf/FastMalloc.cpp:74

> 
> whoops

Done

>> Source/JavaScriptCore/wtf/FastMalloc.cpp:83
>> +#if OS(WINDOWS)
> 
> and chromium?
> 
> I think this already compiles fine on Windows for Apple's port. (Ditto for the OS(WINDOWS) below).

Maybe !USE(PTHREADS)? I'll try to figure out the right set of defines
Comment 6 Dmitry Lomov 2011-07-27 16:56:59 PDT
Created attachment 102204 [details]
CR feedback 

Fixed issues.
I couldn't figure the better condition in FastMalloc.cpp than OS(WINDOWS) && PLATFORM(CHROMIUM).

Chromium trybots are happy.
Comment 7 David Levin 2011-07-27 17:00:55 PDT
Comment on attachment 102204 [details]
CR feedback 

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

> Source/JavaScriptCore/wtf/FastMalloc.cpp:87
> +#endif // OS(WINDOWS)

You don't need to have these comments on the endif (and they are incorrect now anyway).

> Source/JavaScriptCore/wtf/FastMalloc.cpp:113
> +static const LPVOID kTlsAllowValue = reinterpret_cast<LPVOID>(0); // must be zero

Ideally make the comment look like a sentence.
"Must be zero."
Comment 8 David Levin 2011-07-27 17:05:24 PDT
And move over wtfThreadData() as you mentioned.
Comment 9 Dmitry Lomov 2011-07-27 17:13:10 PDT
Created attachment 102205 [details]
CR feedback + wtfThreadData
Comment 10 WebKit Review Bot 2011-07-27 22:16:37 PDT
Comment on attachment 102205 [details]
CR feedback + wtfThreadData

Clearing flags on attachment: 102205

Committed r91906: <http://trac.webkit.org/changeset/91906>
Comment 11 WebKit Review Bot 2011-07-27 22:16:42 PDT
All reviewed patches have been landed.  Closing bug.