[webkit-reviews] review denied: [Bug 73309] [Qt] GC should be parallel on Qt platform : [Attachment 118189] This patch results some speed-ups on QT platform

[bugzilla-daemon at webkit.org]

Zoltan Herczeg <zherczeg at webkit.org> has denied Roland Takacs
<Takacs.Roland at stud.u-szeged.hu>'s request for review:
Bug 73309: [Qt] GC should be parallel on Qt platform
https://bugs.webkit.org/show_bug.cgi?id=73309

Attachment 118189: This patch results some speed-ups on QT platform
https://bugs.webkit.org/attachment.cgi?id=118189&action=review

------- Additional Comments from Zoltan Herczeg <zherczeg at webkit.org>
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?