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 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 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?
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 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.
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 %]
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 %]
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 %]
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 %]
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 %]
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()'.
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 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.
>
> > 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
(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 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.
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 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.
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.
(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 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?
(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.
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.
2011-11-30 01:43 PST, Roland Takacs
2011-12-07 02:09 PST, Roland Takacs
zherczeg: commit-queue-
2011-12-09 07:41 PST, Roland Takacs
2011-12-22 02:31 PST, Roland Takacs
2011-12-22 06:05 PST, Roland Takacs
2012-01-02 01:42 PST, Roland Takacs
2012-01-02 05:50 PST, Roland Takacs
2012-01-02 05:53 PST, Roland Takacs
2012-01-03 01:19 PST, Roland Takacs
2012-01-03 02:24 PST, Roland Takacs
2012-01-04 04:57 PST, Roland Takacs
2012-01-05 01:38 PST, Roland Takacs
2012-01-11 05:08 PST, Roland Takacs
2012-01-12 05:42 PST, Roland Takacs
2012-01-13 02:35 PST, Roland Takacs
2012-01-17 00:35 PST, Roland Takacs
2012-01-17 02:30 PST, Roland Takacs
rtakacs: commit-queue-
2012-01-18 08:20 PST, Roland Takacs
2012-01-20 05:34 PST, Roland Takacs
2012-01-20 06:52 PST, Roland Takacs
rtakacs: commit-queue-
2012-01-20 07:22 PST, Roland Takacs
2012-01-22 23:24 PST, Roland Takacs
2012-01-23 01:42 PST, Roland Takacs
2012-01-24 23:59 PST, Roland Takacs
2012-01-25 02:21 PST, Roland Takacs
2012-01-26 00:44 PST, Roland Takacs
2012-02-03 03:21 PST, Roland Takacs