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.
Hey, Qt instead of QT since QT is QuickTime, I changed the topic.
Created attachment 117142 [details] proposed patch
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 117142 [details] proposed patch I will fix and test it.
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 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 Attachment 118576 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10827065
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 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.
(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 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
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 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 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 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
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 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 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 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 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.
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 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
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 120876 [details] This file includes measurements with and without "gc()" call
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.
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 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.
Created attachment 120921 [details] This patch results some speed-ups on QT platform.
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 121241 [details] Undefined UNUSED_PARAM resolved I included the 'UnusedParam.h' in my file to see the UNUSED_PARAM definition...
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
Created attachment 122009 [details] working progress test It is a working progress test, its purpose is whether it builds on Windows platform...
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
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.
Created attachment 122403 [details] Fixed the 'Patch does not apply' problem
Created attachment 122728 [details] There is some change in xproj and NumberOfCores files
Created attachment 122739 [details] fixed the unresolved external symbol problem 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?
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 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.
Created attachment 123308 [details] final update
Created attachment 123312 [details] Final update I forgot to include threadspecific.h , but this patch fixes this problem
Created attachment 123523 [details] working progress test
Created attachment 123529 [details] working progress test
Created attachment 123893 [details] WIP patch
Created attachment 123903 [details] FINAL PATCH
Comment on attachment 123903 [details] FINAL PATCH For what it's worth, this looks right to me!
(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.)
Created attachment 124070 [details] Final update I moved ' #include <wtf/ThreadSpecific.h> ' to the MainThread.cpp
Comment on attachment 124070 [details] Final update r=me
Comment on attachment 124070 [details] Final update Clearing flags on attachment: 124070 Committed r105982: <http://trac.webkit.org/changeset/105982>
All reviewed patches have been landed. Closing bug.
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.
Reopen, because it was rolled out - http://trac.webkit.org/changeset/105987
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 on attachment 125302 [details] Final update r=me
Comment on attachment 125302 [details] Final update Clearing flags on attachment: 125302 Committed r107390: <http://trac.webkit.org/changeset/107390>
You added a few "#if PLATFORM(WINDOWS)". Did you mean "#if PLATFORM(WIN)" or is the change correct?