Bug 130288 - It should be possible to adjust DFG and FTL compiler thread priorities
Summary: It should be possible to adjust DFG and FTL compiler thread priorities
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-14 22:53 PDT by Michael Saboff
Modified: 2014-06-27 14:59 PDT (History)
4 users (show)

See Also:


Attachments
Patch (9.72 KB, patch)
2014-03-14 23:24 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Updated patch for landing (9.94 KB, patch)
2014-03-15 09:13 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2014-03-14 22:53:51 PDT
On systems with 1 or 2 cores, the DFG and FTL compiler threads might compete with each other or the main thread.  By lowering the thread priorities of the compilation threads, the main thread would get higher priority.  Given the nature of the DFG and FTL, it makes sense to elevate the DFG compiler threads above the FTL compiler threads.
Comment 1 Michael Saboff 2014-03-14 23:24:21 PDT
Created attachment 226811 [details]
Patch
Comment 2 Geoffrey Garen 2014-03-15 00:47:03 PDT
Comment on attachment 226811 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226811&action=review

> Source/WTF/ChangeLog:10
> +        pools.  For two core systems, there might be thread runnable threads, the main thread,

I think you meant "three" and not "thread".

> Source/WTF/ChangeLog:14
> +        compilation threads, the main thread can run.  Further tests may suggest better values
> +        for the new options, priorityDeltaOfDFGCompilerThreads and priorityDeltaOfFTLCompilerThreads.

What are the consequences of the values in this patch? We don't usually make performance changes without comment on benchmark effects.

> Source/WTF/wtf/ThreadingPthreads.cpp:207
> +void changedThreadPriority(ThreadIdentifier threadID, int delta)

I think you meant "changeThreadPriority".

> Source/WTF/wtf/ThreadingWin.cpp:249
> +void changedThreadPriority(ThreadIdentifier threadID, int delta)

Ditto.
Comment 3 Filip Pizlo 2014-03-15 07:24:31 PDT
Comment on attachment 226811 [details]
Patch

R=me, but what Geoff said.
Comment 4 Michael Saboff 2014-03-15 09:13:37 PDT
Created attachment 226823 [details]
Updated patch for landing

Made suggested changes.  Fixed windows build.  Added performance benefit of the change to the ChangeLogs.

This changes gives a net performance improvement between 1-3% on a 2 core device across SunSpider, Octane, Kraken and AsmBench.
Comment 5 Geoffrey Garen 2014-03-19 09:41:46 PDT
This patch was wrong because it set the priority of pthread_self() instead of the passed-in thread. It was fixed in <http://trac.webkit.org/changeset/165691>, netting a much bigger speedup.
Comment 6 Csaba Osztrogonác 2014-06-04 03:30:21 PDT
Comment on attachment 226811 [details]
Patch

Cleared Filip Pizlo's review+ from obsolete attachment 226811 [details] so that this bug does not appear in http://webkit.org/pending-commit.