RESOLVED FIXED Bug 73309
[Qt] GC should be parallel on Qt platform
https://bugs.webkit.org/show_bug.cgi?id=73309
Summary [Qt] GC should be parallel on Qt platform
Roland Takacs
Reported 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.
Attachments
proposed patch (13.43 KB, patch)
2011-11-30 01:43 PST, Roland Takacs
no flags
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-
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-
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-
This patch results some speed-ups on QT platform. (20.25 KB, patch)
2011-12-22 06:05 PST, Roland Takacs
gustavo: commit-queue-
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-
This patch results some speed-ups on QT platform. (20.56 KB, patch)
2012-01-02 05:50 PST, Roland Takacs
no flags
This file includes measurements with and without "gc()" call (983 bytes, text/plain)
2012-01-02 05:53 PST, Roland Takacs
no flags
This patch results some speed-ups on QT platform. (20.52 KB, patch)
2012-01-03 01:19 PST, Roland Takacs
no flags
This patch results some speed-ups on QT platform. (20.55 KB, patch)
2012-01-03 02:24 PST, Roland Takacs
no flags
fixed the windows build problem (21.15 KB, patch)
2012-01-04 04:57 PST, Roland Takacs
no flags
Undefined UNUSED_PARAM resolved (21.18 KB, patch)
2012-01-05 01:38 PST, Roland Takacs
webkit.review.bot: commit-queue-
working progress test (16.79 KB, patch)
2012-01-11 05:08 PST, Roland Takacs
webkit.review.bot: commit-queue-
fixed the unresolved external symbol problem (21.42 KB, patch)
2012-01-12 05:42 PST, Roland Takacs
no flags
Fixed the 'Patch does not apply' problem (22.08 KB, patch)
2012-01-13 02:35 PST, Roland Takacs
no flags
There is some change in xproj and NumberOfCores files (22.31 KB, patch)
2012-01-17 00:35 PST, Roland Takacs
no flags
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-
This patch do the parallel gc only (6.09 KB, patch)
2012-01-18 08:20 PST, Roland Takacs
kbalazs: review-
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
final update (5.43 KB, patch)
2012-01-20 06:52 PST, Roland Takacs
rtakacs: review-
rtakacs: commit-queue-
Final update (5.40 KB, patch)
2012-01-20 07:22 PST, Roland Takacs
no flags
working progress test (4.23 KB, patch)
2012-01-22 23:24 PST, Roland Takacs
no flags
working progress test (4.48 KB, patch)
2012-01-23 01:42 PST, Roland Takacs
no flags
WIP patch (2.60 KB, patch)
2012-01-24 23:59 PST, Roland Takacs
no flags
FINAL PATCH (4.11 KB, patch)
2012-01-25 02:21 PST, Roland Takacs
no flags
Final update (3.83 KB, patch)
2012-01-26 00:44 PST, Roland Takacs
no flags
Final update (6.19 KB, patch)
2012-02-03 03:21 PST, Roland Takacs
no flags
Zoltan Horvath
Comment 1 2011-11-30 00:42:11 PST
Hey, Qt instead of QT since QT is QuickTime, I changed the topic.
Roland Takacs
Comment 2 2011-11-30 01:43:42 PST
Created attachment 117142 [details] proposed patch
Gabor Loki
Comment 3 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.
Roland Takacs
Comment 4 2011-12-01 06:41:31 PST
Comment on attachment 117142 [details] proposed patch I will fix and test it.
Roland Takacs
Comment 5 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]
Zoltan Herczeg
Comment 6 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?
Roland Takacs
Comment 7 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]
Collabora GTK+ EWS bot
Comment 8 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
Gyuyoung Kim
Comment 9 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
Balazs Kelemen
Comment 10 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.
Balazs Kelemen
Comment 11 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.
WebKit Review Bot
Comment 12 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
Roland Takacs
Comment 13 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 %]
Gyuyoung Kim
Comment 14 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
WebKit Review Bot
Comment 15 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
Gustavo Noronha (kov)
Comment 16 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
Roland Takacs
Comment 17 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 %]
Gustavo Noronha (kov)
Comment 18 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
WebKit Review Bot
Comment 19 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
Gyuyoung Kim
Comment 20 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
Raphael Kubo da Costa (:rakuco)
Comment 21 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.
Roland Takacs
Comment 22 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 %]
Gyuyoung Kim
Comment 23 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
Roland Takacs
Comment 24 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 %]
Roland Takacs
Comment 25 2012-01-02 05:53:18 PST
Created attachment 120876 [details] This file includes measurements with and without "gc()" call
Balazs Kelemen
Comment 26 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.
Roland Takacs
Comment 27 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 %]
Kenneth Rohde Christiansen
Comment 28 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.
Roland Takacs
Comment 29 2012-01-03 02:24:35 PST
Created attachment 120921 [details] This patch results some speed-ups on QT platform.
Roland Takacs
Comment 30 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()'.
Roland Takacs
Comment 31 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...
WebKit Review Bot
Comment 32 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
Roland Takacs
Comment 33 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...
WebKit Review Bot
Comment 34 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
Roland Takacs
Comment 35 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.
Roland Takacs
Comment 36 2012-01-13 02:35:42 PST
Created attachment 122403 [details] Fixed the 'Patch does not apply' problem
Roland Takacs
Comment 37 2012-01-17 00:35:39 PST
Created attachment 122728 [details] There is some change in xproj and NumberOfCores files
Roland Takacs
Comment 38 2012-01-17 02:30:06 PST
Created attachment 122739 [details] fixed the unresolved external symbol problem on Windows platform
Zoltan Horvath
Comment 39 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.
Balazs Kelemen
Comment 40 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
Zoltan Horvath
Comment 41 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. :)
Zoltan Herczeg
Comment 42 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.
Balazs Kelemen
Comment 43 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?
Roland Takacs
Comment 44 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.
Balazs Kelemen
Comment 45 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.
Roland Takacs
Comment 46 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.
Roland Takacs
Comment 47 2012-01-20 06:52:19 PST
Created attachment 123308 [details] final update
Roland Takacs
Comment 48 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
Roland Takacs
Comment 49 2012-01-22 23:24:58 PST
Created attachment 123523 [details] working progress test
Roland Takacs
Comment 50 2012-01-23 01:42:34 PST
Created attachment 123529 [details] working progress test
Roland Takacs
Comment 51 2012-01-24 23:59:48 PST
Created attachment 123893 [details] WIP patch
Roland Takacs
Comment 52 2012-01-25 02:21:15 PST
Created attachment 123903 [details] FINAL PATCH
Filip Pizlo
Comment 53 2012-01-25 02:25:46 PST
Comment on attachment 123903 [details] FINAL PATCH For what it's worth, this looks right to me!
Filip Pizlo
Comment 54 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.)
Roland Takacs
Comment 55 2012-01-26 00:44:52 PST
Created attachment 124070 [details] Final update I moved ' #include <wtf/ThreadSpecific.h> ' to the MainThread.cpp
Zoltan Herczeg
Comment 56 2012-01-26 00:48:38 PST
Comment on attachment 124070 [details] Final update r=me
WebKit Review Bot
Comment 57 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>
WebKit Review Bot
Comment 58 2012-01-26 02:22:49 PST
All reviewed patches have been landed. Closing bug.
Nikolas Zimmermann
Comment 59 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?
Nikolas Zimmermann
Comment 60 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.
Csaba Osztrogonác
Comment 61 2012-01-26 04:30:24 PST
Reopen, because it was rolled out - http://trac.webkit.org/changeset/105987
Roland Takacs
Comment 62 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.
Zoltan Herczeg
Comment 63 2012-02-09 23:59:55 PST
Comment on attachment 125302 [details] Final update r=me
WebKit Review Bot
Comment 64 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>
WebKit Review Bot
Comment 65 2012-02-10 02:49:19 PST
All reviewed patches have been landed. Closing bug.
Patrick R. Gansterer
Comment 66 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?
Note You need to log in before you can comment on or make changes to this bug.