[Webkit-unassigned] [Bug 73309] [Qt] GC should be parallel on Qt platform

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73309


Balazs Kelemen <kbalazs at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #118576|commit-queue-               |commit-queue?
               Flag|                            |




--- Comment #10 from Balazs Kelemen <kbalazs at webkit.org>  2011-12-09 08:07:43 PST ---
(From update of attachment 118576)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.