Closed Bug 1416986 Opened 2 years ago Closed 2 years ago

[e10s a11y] Reduce cross-process QueryInterface using handler payload

Categories

(Core :: Disability Access APIs, defect)

All
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- fixed
firefox59 --- verified

People

(Reporter: Jamie, Assigned: Jamie)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files)

Now that virtual buffers have to render across processes, we want to eliminate as many cross-process calls as possible. This includes QueryInterface calls, since buffers query for several interfaces on every node they visit.

Bug 1414497 explores doing this using IMultiQI. However, this still requires us to make one extra cross-process call after we receive the initial interface. Instead, this bug eliminates this extra call and instead includes the interface pointers in the handler payload.

This assumes that COM will aggregate the interfaces in the payload into the proxy manager such that future QI calls on the proxy will resolve them locally.
Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d33c0e227313c04ed996633897d0b3724e88b22

Marco, I'd love your thoughts on this. In particular, it'd be interesting to know not only if perf is better than nightly (it seems to be locally), but also how it compares to the build from bug 1414497. In theory, this second approach should be slightly faster, assuming I'm not missing something (which is entirely possible).
let's CC Marco this time. :)

(from comment #1)
> Try build:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1d33c0e227313c04ed996633897d0b3724e88b22
> 
> Marco, I'd love your thoughts on this. In particular, it'd be interesting to
> know not only if perf is better than nightly (it seems to be locally), but
> also how it compares to the build from bug 1414497. In theory, this second
> approach should be slightly faster, assuming I'm not missing something
> (which is entirely possible).
(In reply to James Teh [:Jamie] from comment #1)
> Marco, I'd love your thoughts on this. In particular, it'd be interesting to
> know not only if perf is better than nightly (it seems to be locally), but
> also how it compares to the build from bug 1414497. In theory, this second
> approach should be slightly faster, assuming I'm not missing something
> (which is entirely possible).

This build is faster than both Nightly and the try build from bug 1414497. WW I loads in under 7 seconds, Twitter is usable with this build, Slack and IRCCloud are both very snappy, also when typing and sending off messages, the braille display reacts much faster than in both other builds.
Try build with the updated patches (which clean things up and cache yet another interface):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85bc5e7c6cac0bd2bed74774cc286c255a87fb0d
Blocks: 1417327
Comment on attachment 8928369 [details]
Bug 1416986 part 1: Allow an mscom Handler to signal that it knows an interface is definitely not available.

https://reviewboard.mozilla.org/r/199580/#review206070
Attachment #8928369 - Flags: review?(aklotz) → review+
Comment on attachment 8928370 [details]
Bug 1416986 part 2: Include interfaces the client is likely to request in the accessible handler payload.

https://reviewboard.mozilla.org/r/199582/#review206072

::: accessible/ipc/win/HandlerProvider.cpp:28
(Diff revision 1)
>  #include "mozilla/mscom/FastMarshaler.h"
>  #include "mozilla/mscom/MainThreadInvoker.h"
>  #include "mozilla/mscom/Ptr.h"
>  #include "mozilla/mscom/StructStream.h"
>  #include "mozilla/mscom/Utils.h"
> +#include "mozilla/mscom/Interceptor.h"

Nit: Please move this include so that it is in alphabetical order with respect to the others.

::: accessible/ipc/win/HandlerProvider.cpp:215
(Diff revision 1)
> +    return;
> +  }
> +
> +  // Some of these interfaces aren't present on all accessibles,
> +  // so it's not a failure if these interfaces can't be fetched.
> +  hr = aInterceptor->GetInterceptorForIID(IID_IEnumVARIANT,

We're going to need to release the references in StaticIA2Data once they have been marshaled.

When CoMarshalInterface writes the interface out to a stream, it AddRef()s. After that has occurred, you should release the interfaces that are in StaticIA2Data before you destroy it.

::: accessible/ipc/win/handler/AccessibleHandler.cpp:253
(Diff revision 1)
> +  // These interfaces have been aggregated into the proxy manager.
> +  // The proxy manager will resolve these interfaces now on QI,
> +  // so we can release these pointers.
> +  // Note that if pointers to other objects (in contrast to
> +  // interfaces of *this* object) are added in future, we should not release
> +  // those pointers.

Fantastic comment, thank you!

::: accessible/ipc/win/handler/HandlerData.idl:51
(Diff revision 1)
>    BSTR              mDescription;
>    BSTR              mDefaultAction;
>    BSTR              mValue;
>    BSTR              mAttributes;
>    IA2Locale         mIA2Locale;
>    long              mUniqueId;

Please file a follow-up bug to move some of these fields to StaticIA2Data.

::: accessible/ipc/win/handler/HandlerData.idl:122
(Diff revision 1)
>  {
>    typedef struct _IA2Payload
>    {
> -    IA2Data mData;
> +    StaticIA2Data mStaticData;
> +    DynamicIA2Data mDynamicData;
>      IGeckoBackChannel* mGeckoBackChannel;

Could you please file a follow-up bug to move mGeckoBackChannel into StaticIA2Data?
Attachment #8928370 - Flags: review?(aklotz) → review-
Comment on attachment 8928371 [details]
Bug 1416986 part 3: AccessibleHandler: Avoid cross-process QI calls for interfaces which we know don't exist.

https://reviewboard.mozilla.org/r/199584/#review206078

::: accessible/ipc/win/handler/AccessibleHandler.cpp:274
(Diff revision 1)
>    }
>    if (deserializer.IsEmpty()) {
>      return S_FALSE;
>    }
>  
> -  if (!deserializer.Read(&mCachedData, &IA2Payload_Decode)) {
> +  // QueryHandlerInterface might get called while we dserialize the payload,

Nit: s/dserialize/deserialize

::: accessible/ipc/win/handler/AccessibleHandler.cpp:278
(Diff revision 1)
>  
> -  if (!deserializer.Read(&mCachedData, &IA2Payload_Decode)) {
> +  // QueryHandlerInterface might get called while we dserialize the payload,
> +  // but that checks the interface pointers in the payload to determine what
> +  // interfaces are available. Therefore, deserialize into a temporary struct
> +  // and update mCachedData only after deserialization completes.
> +  IA2Payload newData;

s/newData/newData{}/

The payload decoding functions tend to act up when their target memory is not zeroed out beforehand, so let's make sure that we do that for newData.
Attachment #8928371 - Flags: review?(aklotz) → review+
Comment on attachment 8928372 [details]
Bug 1416986 part 4: AccessibleHandler: Don't fall through to the proxy for IAccessibleHyperlink.

https://reviewboard.mozilla.org/r/199586/#review206080
Attachment #8928372 - Flags: review?(aklotz) → review+
My main concern with part 2 is the cleanup of StaticIA2Data.
The updates to parts 1 and 4 were just due to a rebase.
Blocks: 1418887
(In reply to Aaron Klotz [:aklotz] from comment #10)
Filed bug 1418887 to deal with moving items to StaticIA2Data.
Comment on attachment 8928370 [details]
Bug 1416986 part 2: Include interfaces the client is likely to request in the accessible handler payload.

https://reviewboard.mozilla.org/r/199582/#review206564

::: accessible/ipc/win/HandlerProvider.cpp:133
(Diff revision 2)
>    mSerializer = MakeUnique<mscom::StructToStream>(payload, &IA2Payload_Encode);
>  
> -  // Now that we have serialized payload, we should free any BSTRs that were
> -  // allocated in BuildIA2Data.
> -  ClearIA2Data(payload.mData);
> +  // Now that we have serialized payload, we should clean up any
> +  // BSTRs, interfaces, etc. fetched in BuildInitialIA2Data.
> +  CleanupStaticIA2Data(payload.mStaticData);
> +  ClearDynamicIA2Data(payload.mDynamicData);

Should we sync these function names? ie, s/Clear/Cleanup to match the static variant?

::: accessible/ipc/win/HandlerProvider.cpp:387
(Diff revision 2)
> +  if (aData.mIATable2) {
> +    aData.mIATable2->Release();
> +  }
> +  if (aData.mIATableCell) {
> +    aData.mIATableCell->Release();
> +  }

How about we zero out the struct at the end of this?
Attachment #8928370 - Flags: review?(aklotz) → review+
Keywords: checkin-needed
Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5d45b7151b5c
part 1: Allow an mscom Handler to signal that it knows an interface is definitely not available. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/0a2081375228
part 2: Include interfaces the client is likely to request in the accessible handler payload. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/3c63a44aa69c
part 3: AccessibleHandler: Avoid cross-process QI calls for interfaces which we know don't exist. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/edd996f2588c
part 4: AccessibleHandler: Don't fall through to the proxy for IAccessibleHyperlink. r=aklotz
Keywords: checkin-needed
Fix verified in build 59.0a1 (20171122103138).
Depends on: 1419814
Comment on attachment 8928369 [details]
Bug 1416986 part 1: Allow an mscom Handler to signal that it knows an interface is definitely not available.

Approval Request Comment
[Feature/Bug causing the regression]: Windows e10s accessibility.
[User impact if declined]: Continued poor performance of Windows accessibility.
[Is this code covered by automated tests?]: No, because there is no mechanism for platform accessibility testing.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Bug 1413287.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Straightforward patch which only affects accessibility.
[String changes made/needed]: None.
Attachment #8928369 - Flags: approval-mozilla-beta?
Comment on attachment 8928370 [details]
Bug 1416986 part 2: Include interfaces the client is likely to request in the accessible handler payload.

Approval Request Comment
[Feature/Bug causing the regression]: Windows e10s accessibility.
[User impact if declined]: Continued poor performance of Windows accessibility.
[Is this code covered by automated tests?]: No, because there is no mechanism for platform accessibility testing.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Bug 1413287, other patches from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only affects accessibility. Accessibility performance is unusable for many use cases in 57, so we can't make things much worse.
[String changes made/needed]: None.
Attachment #8928370 - Flags: approval-mozilla-beta?
Comment on attachment 8928371 [details]
Bug 1416986 part 3: AccessibleHandler: Avoid cross-process QI calls for interfaces which we know don't exist.

Approval Request Comment
[Feature/Bug causing the regression]: Windows e10s accessibility.
[User impact if declined]: Continued poor performance of Windows accessibility.
[Is this code covered by automated tests?]: No, because there is no mechanism for platform accessibility testing.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Bug 1413287, other patches from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only affects accessibility. Accessibility performance is unusable for many use cases in 57, so we can't make things much worse.
[String changes made/needed]: None.
Attachment #8928371 - Flags: approval-mozilla-beta?
Comment on attachment 8928372 [details]
Bug 1416986 part 4: AccessibleHandler: Don't fall through to the proxy for IAccessibleHyperlink.

Approval Request Comment
[Feature/Bug causing the regression]: Windows e10s accessibility.
[User impact if declined]: Continued poor performance of Windows accessibility.
[Is this code covered by automated tests?]: No, because there is no mechanism for platform accessibility testing.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Bug 1413287, other patches from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only affects accessibility. Straightforward patch for consistency/correctness.
[String changes made/needed]: None.
Attachment #8928372 - Flags: approval-mozilla-beta?
Hi :Jamie, 
Do we really have to take this in Beta58? These are huge patches to me and it seems change a lot. Can we let it ride the train on 59 and bake more?
Flags: needinfo?(jteh)
(In reply to Gerry Chang [:gchang] from comment #31)
> Do we really have to take this in Beta58? These are huge patches to me and
> it seems change a lot. Can we let it ride the train on 59 and bake more?

I can certainly understand why this is a scary uplift request. However, performance for Windows accessibility was *severely* regressed in 57 (~13x reduction in speed in some cases). It is not practical to use at all for some use cases and we have lost some users as a result. The patches certainly aren't small, but their scope is limited to accessibility (ipc/mscom is only used by accessibility at this stage). I respect that release management must consider the risk to the release as a whole, but I hope that the severity of the situation with accessibility will be given due consideration in the decision making process.

NI davidb for awareness.
Flags: needinfo?(jteh) → needinfo?(dbolter)
In addition to what Jamie said, I have been running try builds with these patches for a week in a productive fashion, giving them exposure to a lot of stuff I use every day, and also tested out several sites to compare performance. As can be seen from bug 1418290, SuMo and other channels, Jamie is not exagerating with his statements regarding the regressing of users of accessibility in 57. And while 58 is just slightly better, it is not nearly at a point to win back users. And the more versions we let them down, the less likely it will be that they come back.
Given this unusual situation I want to see this land for reasons given in comment 32 and comment 33, but I'd like it to land very soon so we get more beta testing time. I have full confidence Jamie will back it out if there are problems.
Flags: needinfo?(dbolter)
Comment on attachment 8928369 [details]
Bug 1416986 part 1: Allow an mscom Handler to signal that it knows an interface is definitely not available.

In order to improve the performance for Windows accessibility and win users back. Let's take it in Beta58. Since it's limited to accessibility, if there are any issues, we can alwasy back them out. Beta58+.
Attachment #8928369 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8928370 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8928371 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8928372 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.