1627084 - Build handler payloads without extra cross-thread calls for IGeckoBackChannel bulk fetch methods

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement, P3)

Description

[James Teh [:Jamie]]

HandlerProvider currently implements two methods on IGeckoBackChannel which bulk fetch objects to avoid extra cross-process calls: get_AllTextInfo and get_AllChildren.

1. These methods are called on an MTA thread, but dispatch most of their work to the main thread.
2. In the main thread, any objects to be returned must be (and are) wrapped in Interceptors.
3. Back in the MTA thread, when COM marshals the result of these calls back to the client, it marshals each returned object, but that requires building handler payloads for each one.
4. Since handler payloads have to be built on the main thread, that means we make a separate cross-thread call for each object.

If there are a lot of objects, this means quite a lot of cross-thread calls, incurring unnecessary overhead.

Instead, we can build the payload for the object as we wrap it in the Interceptor (step 2 above). That means these bulk fetch calls only require one cross-thread call.

Refreshing the NVDA buffer for a simple 7000 item list takes ~4.3 sec on Nightly. With my WIP patch for this, it takes ~3.9 sec, a ~9.3% decrease.

This is tricky to get right for a couple of reasons:

1. There's currently no way to get a HandlerProvider from an Interceptor. In my WIP patch, I've added methods to do this, but it's somewhat ugly with a few static_casts.
2. If multiple callers simultaneously access the same object, I think we might run into race conditions. We have a mutex on the serializer, but I don't think we can use that here because of potential deadlocks.

Comment 1

[James Teh [:Jamie]]

Here's why I don't think we can use HandlerProvider::mMutex for the pre-built payload:

1. MTA thread 1 makes a bulk fetch call. It gets dispatched to the main thread.
2. MTA thread 2 retrieves object o. It needs the payload for o, so it acquires the mutex. It then queues a call to the main thread to build the payload.
3. In the main thread, the bulk fetch call needs to build a payload for o. It tries to acquire the mutex.
4. Unfortunately, the mutex is already being held by MTA thread 2, which is waiting on the main thread to process its request to build the payload. Deadlock!

Comment 2

[James Teh [:Jamie]]

Attachment: Bug 1627084 part 2: mscom: Provide access to the HandlerProvider from the Interceptor.

Because MainThreadHandoff sits between the Interceptor and the HandlerProvider, the caller must:

1. Get the event sink (the MainThreadhandoff) from the Interceptor; and
2. Get the HandlerProvider from the MainThreadHandoff.

Because IInterceptorSink has no knowledge of HandlerProviders, the caller has to static_cast the IInterceptorSink from step 1 to MainThreadHandoff.

Comment 3

[James Teh [:Jamie]]

Attachment: Bug 1627084 part 3: Build handler payloads in HandlerProvider::ToWrappedObject.

This avoids separate cross-thread calls when marshaling each object returned from an IGeckoBackChannel bulk fetch method.

Comment 4

[James Teh [:Jamie]]

Attachment: Bug 1627084 part 1: Don't compile ipc/mscom/MainThreadHandoff.cpp in unified mode.

We need to define INITGUID here in a subsequent patch, which is incompatible with unified mode.

Comment 5

[James Teh [:Jamie]]

Attachment: Bug 1627084 part 4: Tidy up functions which clean up IA2 payload data.

1. CleanupStaticIA2Data just called ReleaseStaticIA2DataInterfaces and zeroed memory.
   We don't need it to zero memory, since we're going to delete the data anyway.
   So, just call ReleaseStaticIA2DataInterfaces directly and get rid of CleanupStaticIA2Data.
2. CleanupDynamicIA2Data had a aZeroMemory argument, but no caller ever set it to true.
   Therefore, get rid of the argument.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/269ca6f2610c
part 1: Don't compile ipc/mscom/MainThreadHandoff.cpp in unified mode. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/24936cdfa320
part 2: mscom: Provide access to the HandlerProvider from the Interceptor. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/927287985117
part 3: Build handler payloads in HandlerProvider::ToWrappedObject. r=aklotz,MarcoZ
https://hg.mozilla.org/integration/autoland/rev/13b01a46138c
part 4: Tidy up functions which clean up IA2 payload data. r=MarcoZ

Comment 7

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/269ca6f2610c
https://hg.mozilla.org/mozilla-central/rev/24936cdfa320
https://hg.mozilla.org/mozilla-central/rev/927287985117
https://hg.mozilla.org/mozilla-central/rev/13b01a46138c