... according to the number of cores we have in the system.
Created attachment 108171 [details] proposed patch This was tested on Linux and Mac, and the Windows implementation just need to work if MSDN doesn't lie. The other OS-s should work according to http://stackoverflow.com/questions/150355/programmatically-find-the-number-of-cores-on-a-machine but if that's not the case those can be fixed later (since those are not very intensively developed/tested platforms anyway). This implementation determines the logical number of cores. This should to be ok, according to benchmark results. I tested it on Linux with Methanol - the SVG test set - on a 2 physical / 4 logical core machine. Using all the 4 cores was a win: trunk: 2841 ms Parallel Jobs (generic) enabled: 1974 ms PJ enabled + patch: 1770 ms
I think it is a necessary step before enabling Parallel Jobs by default.
Attachment 108171 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/wtf/ParallelJobsGeneric.cpp:36: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 108292 [details] Patch v2 Fixed style + fixed Windows build.
Comment on attachment 108292 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=108292&action=review > Source/JavaScriptCore/ChangeLog:9 > + covers only Linux, AIX, Solaris, Darwin, OpenBSD, NetBSD and > + Windows. The hard coded constant is still used on other OS's Did you test your patch on all of those platforms? If not, please note which one is tested. > Source/JavaScriptCore/wtf/ParallelJobsGeneric.cpp:47 > +unsigned ParallelEnvironment::s_maxNumberOfParallelThreads = unsigned(-1); Well, I do not really like this cast. Why don't we use just a signed integer for this? Are those 32k more cores are really needed ? ;) > Source/JavaScriptCore/wtf/ParallelJobsGeneric.cpp:59 > + int sysctlResult = sysctl(name, 2, &result, &length, 0, 0); We should try to avoid using magic numbers! Could you describe what they mean? > Source/JavaScriptCore/wtf/ParallelJobsGeneric.cpp:69 > + // FIXME: should be implemented for all platforms. It is a fallback case, not a real defect of a FIXME. > Source/JavaScriptCore/wtf/ParallelJobsGeneric.h:47 > + if (s_maxNumberOfParallelThreads == unsigned(-1)) Same as above at the initialization. Well, I though this patch removes the PARALELL_JOBS macros. It would be nice if we were able to remove all compile time conditions. Anyway this patch looks good for me.
Created attachment 108309 [details] Patch v3
> Well, I though this patch removes the PARALELL_JOBS macros. It would be nice if we were able to remove all compile time conditions. I would do this in a next step. It's also depends on #66378. Your other comments has been incorporated into the new patch.
Comment on attachment 108309 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=108309&action=review > Source/JavaScriptCore/wtf/ParallelJobsGeneric.cpp:51 > + const int defaultIfNotAvailable = 2; defaultIfUnavailable > Source/JavaScriptCore/wtf/ParallelJobsGeneric.h:59 > - for (unsigned int i = 0; i < maxParallelThreads && m_threads.size() < maxNewThreads; ++i) { > + for (unsigned int i = 0; i < s_maxNumberOfParallelThreads && m_threads.size() < maxNewThreads; ++i) { Signed unsigned comparison? Surprising that VC does not complain. I think we should change all variables to ints. It is highly unlikely that in the near future we have systems with more than 2 billion cores.
Created attachment 110511 [details] Patch
Comment on attachment 110511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110511&action=review r=me with the followings: > Source/JavaScriptCore/ChangeLog:14 > + on the uncovered OS's which should be fixed in the future. 'the' is not needed > Source/JavaScriptCore/wtf/ParallelJobsGeneric.cpp:51 > + const int defaultIfUnavailable = 2; Unused on Windows. Please put an ifdef guard around it, > Source/JavaScriptCore/wtf/ParallelJobsGeneric.h:59 > + int maxNewThreads = requestedJobNumber - 1; rename to maxNumberOfNewThreads
Committed r97731: <http://trac.webkit.org/changeset/97731>