Bug 73309 - [Qt] GC should be parallel on Qt platform
Summary: [Qt] GC should be parallel on Qt platform
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2011-11-29 04:50 PST by Roland Takacs
Modified: 2012-02-20 09:39 PST (History)
18 users (show)

See Also:


Attachments
proposed patch (13.43 KB, patch)
2011-11-30 01:43 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
This patch results some speed-ups on QT platform (10.58 KB, patch)
2011-12-07 02:09 PST, Roland Takacs
zherczeg: review-
zherczeg: commit-queue-
Details | Formatted Diff | Diff
This patch results some speed-ups on QT platform (14.24 KB, patch)
2011-12-09 07:41 PST, Roland Takacs
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
This patch results some speed-ups on QT platform. (20.03 KB, patch)
2011-12-22 02:31 PST, Roland Takacs
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
This patch results some speed-ups on QT platform. (20.25 KB, patch)
2011-12-22 06:05 PST, Roland Takacs
gns: commit-queue-
Details | Formatted Diff | Diff
This patch results some speed-ups on QT platform. (20.26 KB, patch)
2012-01-02 01:42 PST, Roland Takacs
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
This patch results some speed-ups on QT platform. (20.56 KB, patch)
2012-01-02 05:50 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
This file includes measurements with and without "gc()" call (983 bytes, text/plain)
2012-01-02 05:53 PST, Roland Takacs
no flags Details
This patch results some speed-ups on QT platform. (20.52 KB, patch)
2012-01-03 01:19 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
This patch results some speed-ups on QT platform. (20.55 KB, patch)
2012-01-03 02:24 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
fixed the windows build problem (21.15 KB, patch)
2012-01-04 04:57 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
Undefined UNUSED_PARAM resolved (21.18 KB, patch)
2012-01-05 01:38 PST, Roland Takacs
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
working progress test (16.79 KB, patch)
2012-01-11 05:08 PST, Roland Takacs
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
fixed the unresolved external symbol problem (21.42 KB, patch)
2012-01-12 05:42 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
Fixed the 'Patch does not apply' problem (22.08 KB, patch)
2012-01-13 02:35 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
There is some change in xproj and NumberOfCores files (22.31 KB, patch)
2012-01-17 00:35 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
fixed the unresolved external symbol problem on Windows platform (23.08 KB, patch)
2012-01-17 02:30 PST, Roland Takacs
zherczeg: review-
rtakacs: commit-queue-
Details | Formatted Diff | Diff
This patch do the parallel gc only (6.09 KB, patch)
2012-01-18 08:20 PST, Roland Takacs
kbalazs: review-
Details | Formatted Diff | Diff
I did the modifications requested by Zoltan Herczeg and Balazs Kelemen (6.08 KB, patch)
2012-01-20 05:34 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
final update (5.43 KB, patch)
2012-01-20 06:52 PST, Roland Takacs
rtakacs: review-
rtakacs: commit-queue-
Details | Formatted Diff | Diff
Final update (5.40 KB, patch)
2012-01-20 07:22 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
working progress test (4.23 KB, patch)
2012-01-22 23:24 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
working progress test (4.48 KB, patch)
2012-01-23 01:42 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
WIP patch (2.60 KB, patch)
2012-01-24 23:59 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
FINAL PATCH (4.11 KB, patch)
2012-01-25 02:21 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
Final update (3.83 KB, patch)
2012-01-26 00:44 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
Final update (6.19 KB, patch)
2012-02-03 03:21 PST, Roland Takacs
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Takacs 2011-11-29 04:50:05 PST
I would like to make parallel gc on QT platform, because of the speed-up. It has been implemented already, but only for mac.
This patch is located on http://trac.webkit.org/changeset/98937.

I modified only the specific contents.

This is only an experimental patch to enable shared gc.
Comment 1 Zoltan Horvath 2011-11-30 00:42:11 PST
Hey,

Qt instead of QT since QT is QuickTime, I changed the topic.
Comment 2 Roland Takacs 2011-11-30 01:43:42 PST
Created attachment 117142 [details]
proposed patch
Comment 3 Gabor Loki 2011-11-30 02:07:37 PST
Comment on attachment 117142 [details]
proposed patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        These changes made the parallel gc feature available for non-mac ports.

That is not true. This patch adds Qt support only.
BTW, you have moved the core detection routine from ParallelJobs to WTF, which is OK, but you have forgot to mention this move here.

> Source/JavaScriptCore/ChangeLog:17
> +        * wtf/NumberOfCores.cpp: Added.

Did you add this to the project files? It looks like you missed it.

> Source/JavaScriptCore/ChangeLog:19
> +        * wtf/NumberOfCores.h: Added.

Ditto.

> Source/JavaScriptCore/runtime/Heuristics.cpp:45
> +#include <wtf/NumberOfCores.h>

I guess it will be an alphabetical order issue.

> Source/JavaScriptCore/wtf/NumberOfCores.cpp:61
> +        if (sysctlResult >= 0) {
> +
> +            s_numOfCores = result;
> +
> +        }

Empty lines!
No need for the curly brackets in this case!

> Source/JavaScriptCore/wtf/NumberOfCores.cpp:71
> +        if (sysconfResult >= 0) {
> +
> +            s_numOfCores = static_cast<int>(sysconfResult);
> +
> +        }

Empty lines, curly brackets!



Well, I guess it will fail on several ports. You should consider the project files.
Before sending a patch for review you should make sure do not brake anything.
At least you have to execute run-webkit-test on Qt port.
Comment 4 Roland Takacs 2011-12-01 06:41:31 PST
Comment on attachment 117142 [details]
proposed patch

I will fix and test it.
Comment 5 Roland Takacs 2011-12-07 02:09:07 PST
Created attachment 118189 [details]
This patch results some speed-ups on QT platform

The run-sunspider results:
Total: 183.9ms +/- 3.9%  [before modification]
Total: 151.2ms +/- 0.8%  [after modification]

The run-sunspider --v8-suite results:
Total: 947.5ms +/- 2.6% [before modification]
Total: 775.3ms +/- 6.6% [after modification]
Comment 6 Zoltan Herczeg 2011-12-07 02:27:34 PST
Comment on attachment 118189 [details]
This patch results some speed-ups on QT platform

Nice speedup!

A couple of things:

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

> Source/JavaScriptCore/runtime/Heuristics.cpp:-202
>  } } // namespace JSC::Heuristics
> -
> -

Please don't delete these lines.

> Source/JavaScriptCore/wtf/MainThread.h:54
> +void initializeGCThreads();

Plural? I thought there is only one thread.

> Source/JavaScriptCore/wtf/NumberOfCores.cpp:82
>  #endif
>  
>      }
> -
>      return s_numOfCores;
>  }
>  

No node /trunk/Source/JavaScriptCore/wtf/NumberOfCores.cpp at revision 102229

The system does not know this file...

> Source/JavaScriptCore/wtf/ParallelJobsGeneric.h:133
> +    static Vector< RefPtr<ThreadPrivate> >* s_threadPool;    

Do not add extra whitepsace at the end of a line.

> Source/JavaScriptCore/wtf/Platform.h:1120
> -#if !defined(ENABLE_PARALLEL_GC) && PLATFORM(MAC) && ENABLE(COMPARE_AND_SWAP)
> +#if !defined(ENABLE_PARALLEL_GC) && (PLATFORM(MAC)  || PLATFORM(QT)) && ENABLE(COMPARE_AND_SWAP)

Only one space between (PLATFORM(MAC) || PLATFORM(QT))

> Source/JavaScriptCore/wtf/mac/MainThreadMac.mm:164
>  }
>  #endif
> +*/

Why did you comment out these lines? Shouldn't you remove them?
Comment 7 Roland Takacs 2011-12-09 07:41:36 PST
Created attachment 118576 [details]
This patch results some speed-ups on QT platform

Measurement Results:
*V8           speed-up:  3%  [From: 711.4ms   To: 690.7ms]
*V8 splay     speed-up: 15%  [From: 142.5ms   To: 121.6ms]
*Windscorpion speed-up:  2%  [From: 3353.1ms  To: 3281.6ms]
Comment 8 Collabora GTK+ EWS bot 2011-12-09 07:55:07 PST
Comment on attachment 118576 [details]
This patch results some speed-ups on QT platform

Attachment 118576 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10827065
Comment 9 Gyuyoung Kim 2011-12-09 08:02:21 PST
Comment on attachment 118576 [details]
This patch results some speed-ups on QT platform

Attachment 118576 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10825107
Comment 10 Balazs Kelemen 2011-12-09 08:07:43 PST
Comment on attachment 118576 [details]
This patch results some speed-ups on QT platform

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

Informal review.

> Source/JavaScriptCore/wtf/NumberOfCores.cpp:3
> + * Copyright (C) 2011 University of Szeged
> + * All rights reserved.

Put these to one line, please!

> Source/JavaScriptCore/wtf/NumberOfCores.cpp:48
> +    if (s_numberOfCores < 0) {
> +
> +#if OS(DARWIN) || OS(OPENBSD) || OS(NETBSD)
> +

The preferred style is using early return, like:
if (s_numberOfCores > 0)
    return s_numberOfCores;
...

> Source/JavaScriptCore/wtf/NumberOfCores.cpp:61
> +
> +#elif OS(LINUX) || OS(AIX) || OS(SOLARIS)
> +

no need for the newlines here and before/after the other # directives.

> Source/JavaScriptCore/wtf/NumberOfCores.cpp:76
> +
> +#elif OS(WINDOWS)
> +
> +        UNUSED_PARAM(defaultIfUnavailable);
> +        SYSTEM_INFO sysInfo;
> +        GetSystemInfo(&sysInfo);
> +
> +        s_numberOfCores = sysInfo.dwNumberOfProcessors;
> +
> +#else
> +        s_numberOfCores = 1;

Originally there was a const int named defaultIfUnavailable for the fallback case, please keep it. Actually the UNUSED_PARAM would not build without it. Btw, changing the value to 1 (it used to be 2) is ok, we should not be too optimistic.

> Source/JavaScriptCore/wtf/ParallelJobsGeneric.cpp:58
> +    if (s_maxNumberOfCores == -1)
> +        s_maxNumberOfCores = numberOfCores();

You don't need the static here, you already cache the value in numberOfCores(), so it's ok to call it every time. Btw, it would be better if the cached case of numberOfCores() could be inline.

> Source/JavaScriptCore/wtf/ParallelJobsGeneric.h:93
> -    static int s_maxNumberOfParallelThreads;
> +    static int s_maxNumberOfCores;

You no longer need this.

> Source/JavaScriptCore/wtf/wtf.pro:201
> +    NumberOfCores.cpp \

You should add these at least to the Mac build as well, but maybe the best would be to add it to every build so it would be available cross-platform in the future.
Comment 11 Balazs Kelemen 2011-12-09 08:10:28 PST
(In reply to comment #9)
> (From update of attachment 118576 [details])
> Attachment 118576 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/10825107

You need to guard the initializeGCThreads with ENABLE(PARALLEL_GC), otherwise it's a defined but not used function on !mac && !qt platforms.
Comment 12 WebKit Review Bot 2011-12-09 17:33:18 PST
Comment on attachment 118576 [details]
This patch results some speed-ups on QT platform

Attachment 118576 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10832459
Comment 13 Roland Takacs 2011-12-22 02:31:53 PST
Created attachment 120291 [details]
This patch results some speed-ups on QT platform.

SunSpider measurement results:

V8           speed-up:  1.071x as fast  [From: 746.1ms  To: 696.4ms ] [7 %]
V8 Splay     speed-up:  1.082x as fast  [From: 3490.4ms To: 3226.7ms] [8 %]
WindScorpion speed-up:  1.158x as fast  [From: 145.8ms  To: 125.9ms ] [15,8 %]
Comment 14 Gyuyoung Kim 2011-12-22 02:48:40 PST
Comment on attachment 120291 [details]
This patch results some speed-ups on QT platform.

Attachment 120291 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10986409
Comment 15 WebKit Review Bot 2011-12-22 03:09:50 PST
Comment on attachment 120291 [details]
This patch results some speed-ups on QT platform.

Attachment 120291 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10997421
Comment 16 Gustavo Noronha (kov) 2011-12-22 03:13:10 PST
Comment on attachment 120291 [details]
This patch results some speed-ups on QT platform.

Attachment 120291 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10998395
Comment 17 Roland Takacs 2011-12-22 06:05:55 PST
Created attachment 120313 [details]
This patch results some speed-ups on QT platform.

SunSpider measurement results:

V8           speed-up:  1.071x as fast  [From: 746.1ms  To: 696.4ms ] [7 %]
V8 Splay     speed-up:  1.082x as fast  [From: 3490.4ms To: 3226.7ms] [8 %]
WindScorpion speed-up:  1.158x as fast  [From: 145.8ms  To: 125.9ms ] [15,8 %]
Comment 18 Gustavo Noronha (kov) 2011-12-22 06:45:48 PST
Comment on attachment 120313 [details]
This patch results some speed-ups on QT platform.

Attachment 120313 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10986488
Comment 19 WebKit Review Bot 2011-12-22 06:46:10 PST
Comment on attachment 120313 [details]
This patch results some speed-ups on QT platform.

Attachment 120313 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11000502
Comment 20 Gyuyoung Kim 2011-12-22 06:55:07 PST
Comment on attachment 120313 [details]
This patch results some speed-ups on QT platform.

Attachment 120313 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10981526
Comment 21 Raphael Kubo da Costa (:rakuco) 2011-12-23 05:37:54 PST
Comment on attachment 120313 [details]
This patch results some speed-ups on QT platform.

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

> Source/JavaScriptCore/wtf/NumberOfCores.cpp:48
> +#if OS(DARWIN) || OS(OPENBSD) || OS(NETBSD)

This should work for OS(FREEBSD) as well.
Comment 22 Roland Takacs 2012-01-02 01:42:14 PST
Created attachment 120869 [details]
This patch results some speed-ups on QT platform.

SunSpider measurement results:

V8           speed-up:  1.071x as fast  [From: 746.1ms  To: 696.4ms ] [7 %]
V8 Splay     speed-up:  1.082x as fast  [From: 3490.4ms To: 3226.7ms] [8 %]
WindScorpion speed-up:  1.158x as fast  [From: 145.8ms  To: 125.9ms ] [15,8 %]
Comment 23 Gyuyoung Kim 2012-01-02 02:13:58 PST
Comment on attachment 120869 [details]
This patch results some speed-ups on QT platform.

Attachment 120869 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10924904
Comment 24 Roland Takacs 2012-01-02 05:50:17 PST
Created attachment 120875 [details]
This patch results some speed-ups on QT platform.

SunSpider measurement results:

V8           speed-up:  1.071x as fast  [From: 746.1ms  To: 696.4ms ] [7 %]
V8 Splay     speed-up:  1.082x as fast  [From: 3490.4ms To: 3226.7ms] [8 %]
WindScorpion speed-up:  1.158x as fast  [From: 145.8ms  To: 125.9ms ] [15,8 %]
Comment 25 Roland Takacs 2012-01-02 05:53:18 PST
Created attachment 120876 [details]
This file includes measurements with and without "gc()" call
Comment 26 Balazs Kelemen 2012-01-02 07:02:02 PST
Comment on attachment 120875 [details]
This patch results some speed-ups on QT platform.

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

Informal review.
Some nits, otherwise looks good to me.

> Source/JavaScriptCore/GNUmakefile.list.am:607
> +        Source/JavaScriptCore/wtf/NumberOfCores.h \
> +        Source/JavaScriptCore/wtf/NumberOfCores.cpp \

Nit: wrong identation

> Source/JavaScriptCore/wtf/MainThread.cpp:263
> +#if ENABLE(PARALLEL_GC)
> +static ThreadSpecific<bool>* isGCThread;
> +
> +void initializeGCThreads()
> +{
> +    isGCThread = new ThreadSpecific<bool>();
> +}
> +#endif
> +
> +#if ENABLE(PARALLEL_GC)
> +void registerGCThread()
> +{

Nit: no need for the #endif+#if again, please remove them.

> Source/JavaScriptCore/wtf/NumberOfCores.cpp:46
> +    if (s_numberOfCores > 0)
> +        return s_numberOfCores;

Nit: I would put a newline after that.
Comment 27 Roland Takacs 2012-01-03 01:19:39 PST
Created attachment 120917 [details]
This patch results some speed-ups on QT platform.

SunSpider measurement results:

V8           speed-up:  1.071x as fast  [From: 746.1ms  To: 696.4ms ] [7 %]
WindScorpion speed-up:  1.082x as fast  [From: 3490.4ms To: 3226.7ms] [8 %]
V8 Splay     speed-up:  1.158x as fast  [From: 145.8ms  To: 125.9ms ] [15,8 %]
Comment 28 Kenneth Rohde Christiansen 2012-01-03 01:25:56 PST
Comment on attachment 120917 [details]
This patch results some speed-ups on QT platform.

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

> Source/JavaScriptCore/wtf/NumberOfCores.h:26
> +int numberOfCores();

Why not processorCoreCount? or similar.
Comment 29 Roland Takacs 2012-01-03 02:24:35 PST
Created attachment 120921 [details]
This patch results some speed-ups on QT platform.
Comment 30 Roland Takacs 2012-01-04 04:57:15 PST
Created attachment 121101 [details]
fixed the windows build problem

I added my new files to win build file to build successful on windows, furthermore I took Kenneth's advice, and I renamed my 'NumberOfCores()' function to 'NumberOfProcessorCores()'.
Comment 31 Roland Takacs 2012-01-05 01:38:02 PST
Created attachment 121241 [details]
Undefined UNUSED_PARAM resolved

I included the 'UnusedParam.h' in my file to see the UNUSED_PARAM definition...
Comment 32 WebKit Review Bot 2012-01-05 11:02:01 PST
Comment on attachment 121241 [details]
Undefined UNUSED_PARAM resolved

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

New failing tests:
http/tests/inspector/network/download.html
Comment 33 Roland Takacs 2012-01-11 05:08:41 PST
Created attachment 122009 [details]
working progress test

It is a working progress test, its purpose is whether it builds on Windows platform...
Comment 34 WebKit Review Bot 2012-01-11 05:48:50 PST
Comment on attachment 122009 [details]
working progress test

Attachment 122009 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11167856
Comment 35 Roland Takacs 2012-01-12 05:42:02 PST
Created attachment 122221 [details]
fixed the unresolved external symbol problem

I added the isMainThreadOrGCThread's symbol to the JavaScriptCore.order file to see the 'bool isMainThreadOrGCThread()' function on windows platform.
Comment 36 Roland Takacs 2012-01-13 02:35:42 PST
Created attachment 122403 [details]
Fixed the 'Patch does not apply' problem
Comment 37 Roland Takacs 2012-01-17 00:35:39 PST
Created attachment 122728 [details]
There is some change in xproj and NumberOfCores files
Comment 38 Roland Takacs 2012-01-17 02:30:06 PST
Created attachment 122739 [details]
fixed the unresolved external symbol problem on Windows platform
Comment 39 Zoltan Horvath 2012-01-17 04:37:47 PST
Comment on attachment 122739 [details]
fixed the unresolved external symbol problem on Windows platform

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



> Source/JavaScriptCore/ChangeLog:9
> +        Two files had been added to the project, namely NumberOfCores.h/cpp,

have been

> Source/JavaScriptCore/wtf/MainThread.cpp:253
> +    if (!isGCThread) {

I would write the comment in 1 line before the if and leave the brackets.

> Source/WTF/ChangeLog:8
> +        Two files had been added to the project, namely NumberOfCores.h/cpp,

have been

Otherwise I have no objections.
Comment 40 Balazs Kelemen 2012-01-17 05:12:28 PST
> 
> > Source/JavaScriptCore/wtf/MainThread.cpp:253
> > +    if (!isGCThread) {
> 
> I would write the comment in 1 line before the if and leave the brackets.

Let me nitting you over! ;) I disagree with this because:
1. it is copy-pasted code
2. scoping the comment makes it easier to read, this is the more common practice in WebKit I think
Comment 41 Zoltan Horvath 2012-01-17 23:39:24 PST
(In reply to comment #40)
> Let me nitting you over! ;) I disagree with this because:
> 1. it is copy-pasted code
> 2. scoping the comment makes it easier to read, this is the more common practice in WebKit I think

Right! Then let's see this patch in the trunk. :)
Comment 42 Zoltan Herczeg 2012-01-18 00:53:27 PST
Comment on attachment 122739 [details]
fixed the unresolved external symbol problem on Windows platform

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

> Source/JavaScriptCore/wtf/MainThread.cpp:-92
> -
>  #if !PLATFORM(MAC)
> -

Why did you removed these lines?

> Source/JavaScriptCore/wtf/MainThread.cpp:-107
> -
>  #else
> -

Ditto.

> Source/JavaScriptCore/wtf/MainThread.cpp:255
> +        // This happens if we're running in a process that doesn't care about
> +        // MainThread.

Could you write a better explanation here?

Ok, basically I would split this patch into two:

One which adds the number of processor cores part, and a followup patch which adds the parallel gc feature.
Comment 43 Balazs Kelemen 2012-01-18 02:03:46 PST
Well, I think the main issue that the changelog don't describe the changes in detail so it's hard to review.

(In reply to comment #42)
> (From update of attachment 122739 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122739&action=review
> 
> > Source/JavaScriptCore/wtf/MainThread.cpp:-92
> > -
> >  #if !PLATFORM(MAC)
> > -
> 
> Why did you removed these lines?
> 
> > Source/JavaScriptCore/wtf/MainThread.cpp:-107
> > -
> >  #else
> > -
> 
> Ditto.
> 
> > Source/JavaScriptCore/wtf/MainThread.cpp:255
> > +        // This happens if we're running in a process that doesn't care about
> > +        // MainThread.
> 
> Could you write a better explanation here?

This is copy-pasted code, I don't think the comment should be changed.

> 
> Ok, basically I would split this patch into two:
> 
> One which adds the number of processor cores part, and a followup patch which adds the parallel gc feature.

We already have the number of processor cores part, it has been just moved into WTF to be accessible in JSC. Zoltan, do you agree with me that the patch is ok as it is with a better changelog?
Comment 44 Roland Takacs 2012-01-18 08:20:26 PST
Created attachment 122934 [details]
This patch do the parallel gc only

I took Zoltan Herceg's advice, so I separated to two part my patch. This part contains the parallel gc feature only.
The processor core counter part is located at https://bugs.webkit.org/show_bug.cgi?id=76530.
Comment 45 Balazs Kelemen 2012-01-20 01:21:23 PST
Comment on attachment 122934 [details]
This patch do the parallel gc only

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

I think we need yet another iteration.

> Source/JavaScriptCore/ChangeLog:25
> +        * JavaScriptCore.order:
> +        * JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def:
> +        * wtf/MainThread.cpp:
> +        (WTF::initializeMainThread):
> +        (WTF::registerGCThread):
> +        (WTF::isMainThreadOrGCThread):
> +        * wtf/MainThread.h:
> +        * wtf/Platform.h:
> +        * wtf/mac/MainThreadMac.mm:

Please examine what have you changed and why in a few words.

> Source/JavaScriptCore/wtf/MainThread.cpp:-92
> -
>  #if !PLATFORM(MAC)
> -

Again: why??? Zoltan already pointed out in his review that you should not make such changes.

> Source/JavaScriptCore/wtf/MainThread.cpp:-107
> -
>  #else
> -

Ditto.
Comment 46 Roland Takacs 2012-01-20 05:34:30 PST
Created attachment 123295 [details]
I did the modifications requested by Zoltan Herczeg and Balazs Kelemen

I wrote a few examination to my changes and I did not removed the new lines around the preprocessor macros.
Comment 47 Roland Takacs 2012-01-20 06:52:19 PST
Created attachment 123308 [details]
final update
Comment 48 Roland Takacs 2012-01-20 07:22:38 PST
Created attachment 123312 [details]
Final update

I forgot to include threadspecific.h , but this patch fixes this problem
Comment 49 Roland Takacs 2012-01-22 23:24:58 PST
Created attachment 123523 [details]
working progress test
Comment 50 Roland Takacs 2012-01-23 01:42:34 PST
Created attachment 123529 [details]
working progress test
Comment 51 Roland Takacs 2012-01-24 23:59:48 PST
Created attachment 123893 [details]
WIP patch
Comment 52 Roland Takacs 2012-01-25 02:21:15 PST
Created attachment 123903 [details]
FINAL PATCH
Comment 53 Filip Pizlo 2012-01-25 02:25:46 PST
Comment on attachment 123903 [details]
FINAL PATCH

For what it's worth, this looks right to me!
Comment 54 Filip Pizlo 2012-01-25 02:26:30 PST
(In reply to comment #53)
> (From update of attachment 123903 [details])
> For what it's worth, this looks right to me!

(However I'll let one of the Qt folks decide the final review.)
Comment 55 Roland Takacs 2012-01-26 00:44:52 PST
Created attachment 124070 [details]
Final update

I moved ' #include <wtf/ThreadSpecific.h> ' to the MainThread.cpp
Comment 56 Zoltan Herczeg 2012-01-26 00:48:38 PST
Comment on attachment 124070 [details]
Final update

r=me
Comment 57 WebKit Review Bot 2012-01-26 02:22:40 PST
Comment on attachment 124070 [details]
Final update

Clearing flags on attachment: 124070

Committed r105982: <http://trac.webkit.org/changeset/105982>
Comment 58 WebKit Review Bot 2012-01-26 02:22:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 59 Nikolas Zimmermann 2012-01-26 03:43:51 PST
Comment on attachment 124070 [details]
Final update

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

> Source/JavaScriptCore/wtf/MainThread.cpp:254
> +static ThreadSpecific<bool>* isGCThread;

This breaks the world on mac. isGCThread is still defined in MainThreadMac.mm.
initializeGCThreading creates the one in MainThreadMac.mm, but this ThreadSpecific is never initialized!
I don't get how this can work for you?
Comment 60 Nikolas Zimmermann 2012-01-26 03:50:19 PST
(In reply to comment #59)
> (From update of attachment 124070 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124070&action=review
> 
> > Source/JavaScriptCore/wtf/MainThread.cpp:254
> > +static ThreadSpecific<bool>* isGCThread;
> 
> This breaks the world on mac. isGCThread is still defined in MainThreadMac.mm.
> initializeGCThreading creates the one in MainThreadMac.mm, but this ThreadSpecific is never initialized!
> I don't get how this can work for you?

I suggest you move initializeGCThreading() from MainThreadMac.mm to MainThread.cpp.
But you also have to call them somewhere from the Qt platform code, otherwise it'll still crash random for you - as it does now in trunk with this patch applied.

I'm rolling it out for now.
Comment 61 Csaba Osztrogonác 2012-01-26 04:30:24 PST
Reopen, because it was rolled out - http://trac.webkit.org/changeset/105987
Comment 62 Roland Takacs 2012-02-03 03:21:39 PST
Created attachment 125302 [details]
Final update

Thanks Nikolas to show me what the errors are, so I moved the initializeGCThreads to the MainThread.cpp furthermore I removed the isGCThread from MainThreadMac.mm because MainThread.cpp already contains it.
I created new measurements that I inserted into the Changelog.
Comment 63 Zoltan Herczeg 2012-02-09 23:59:55 PST
Comment on attachment 125302 [details]
Final update

r=me
Comment 64 WebKit Review Bot 2012-02-10 02:49:05 PST
Comment on attachment 125302 [details]
Final update

Clearing flags on attachment: 125302

Committed r107390: <http://trac.webkit.org/changeset/107390>
Comment 65 WebKit Review Bot 2012-02-10 02:49:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 66 Patrick R. Gansterer 2012-02-20 09:39:45 PST
You added a few "#if PLATFORM(WINDOWS)". Did you mean "#if PLATFORM(WIN)" or is the change correct?