1311834 - MSCOM MainThreadInvoker: Spin the background thread while waiting for main thread

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Performance measurements show that the biggest source of perf regression between MTA and STA communication is the cost of blocking the RPC background thread while waiting on the main thread to execute.

As-is, it is possible for the main thread to be blocked (ie, it is in MsgWaitForMultipleObjectsEx) and then the RPC thread blocks waiting for the main thread to wake up and process the APC. Modifying the main thread to poll for messages and APCs instead of blocking fixed the problem. OTOH, we can't have our content process main thread burning up CPU like that. :-)

OTOH, the RPC background thread only waits for a short period of time for the main thread to run its calls. Spinning that thread for such a short time wouldn't be too big a deal.

Subsequent performance tests demonstrate that this brings out-of-process enumeration of all visible accessibles in a gmail tab down to par with non-e10s or better.

Of course, doing this on a uniprocessor box doesn't give us anything, so we'll need to fall back to blocking in that case.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1311834: Make MainThreadInvoker use a spin loop on multiprocessor machines;

Review commit: https://reviewboard.mozilla.org/r/87348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87348/

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Note that with this patch, the entire impetus for bug 1304891 goes away.

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 8803118 [details]
Bug 1311834: Make MainThreadInvoker use a spin loop on multiprocessor machines;

https://reviewboard.mozilla.org/r/87348/#review86596

::: ipc/mscom/MainThreadInvoker.cpp:29
(Diff revision 1)
> +#define CPU_PAUSE() _mm_pause()
> +#elif defined(__GNUC__) || defined(__clang__)
> +#define CPU_PAUSE() __builtin_ia32_pause()
> +#endif
> +
> +static bool sIsMulticore = false;

nit - no need for the false assignment here.

::: ipc/mscom/MainThreadInvoker.cpp:123
(Diff revision 1)
>    PRUint32 tid = ::PR_GetThreadID(mainPrThread);
>    sMainThread = ::OpenThread(SYNCHRONIZE | THREAD_SET_CONTEXT, FALSE, tid);
> +
> +  SYSTEM_INFO sysInfo;
> +  ::GetSystemInfo(&sysInfo);
> +  sIsMulticore = sysInfo.dwNumberOfProcessors > 1;

This is available through NS_SYSTEMINFO_CONTRACTID. Safe to use that here instead?

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 8803118 [details]
Bug 1311834: Make MainThreadInvoker use a spin loop on multiprocessor machines;

https://reviewboard.mozilla.org/r/87348/#review86604

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8803118 [details]
Bug 1311834: Make MainThreadInvoker use a spin loop on multiprocessor machines;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87348/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9839d6c2a187
Make MainThreadInvoker use a spin loop on multiprocessor machines; r=jimm

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9839d6c2a187