Bug 68540 – ParallelJobs: maximum number of threads should be determined dynamically

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Balazs Kelemen]

... according to the number of cores we have in the system.

[Balazs Kelemen]

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

[Balazs Kelemen]

I think it is a necessary step before enabling Parallel Jobs by default.

[WebKit Review Bot]

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.

[Balazs Kelemen]

Created attachment 108292 [details]
Patch v2

Fixed style + fixed Windows build.

[Gabor Loki]

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.

[Balazs Kelemen]

Created attachment 108309 [details]
Patch v3

[Balazs Kelemen]

> 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.

[Zoltan Herczeg]

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.

[Balazs Kelemen]

Created attachment 110511 [details]
Patch

[Zoltan Herczeg]

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

[Balazs Kelemen]

Committed r97731: <http://trac.webkit.org/changeset/97731>