WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
gustavo
: 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
Show Obsolete
(25)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug