1363595 - [e10s a11y] Expand a11y+e10s handler cache

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Based on Trev's comments at https://bugzilla.mozilla.org/show_bug.cgi?id=1321960#c42 we should add low hanging fruit to the COM handler cache.

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking this as affecting 55 based on the original issue.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Do I need to track this? Is the title still accurate or do you want to change it?

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

No tracking yet, this bug is e10s only so we can mark it disabled for 55.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Add time-based expiry to handler cache

This patch adds a timestamp to the cache. I populate it using QueryUnbiasedInterruptTime which takes into account sleep/hibernation (thus preventing being slammed by cache invalidation when the machine wakes up).

On the handler side, we compare the current time with the time saved in the cache, and if it is more than one second old, we refresh.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Cache additional fields

This patch restores the fields that I had been caching when I first wrote the POC. These fields were chosen due to the fact that they are the most frequently requested by NVDA during virtual buffer population.

I intend to expand this as necessary for JAWS as well, but I thought I'd get these in first.

Each API that uses cached values needs to be modified to access them when present.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3: Add invalidation support to AccessibleWrap

The handler supports invalidation. We already track all of the IHandlerControl interfaces that exist, so AccessibleWrap::InvalidateHandlers just does a reverse iteration through those handlers and invalidates each one.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 4: Force invalidation when certain events fire

When an event fires that might affect cached values, we should forcefully invalidate.

I *think* that IsHandlerInvalidationNeeded adequately chooses the subset of events that we need, but I wouldn't mind a second pair of eyes on that. There are some mutation events that I intentionally omitted as we don't cache data related to them.

Comment 8

[James Teh [:Jamie]]

Just a note that this may increase the need for bugs like bug 493683 to be fixed. For example:

data:text/html,<div id="label" style="display: none;">Pause</div><button aria-labelledby="label" onClick="document.getElementById('label').textContent = 'Play';"></button>

Firefox doesn't fire a nameChange event in this case because the change occurred in the labelling element. This is already a problem for our buffers, but at least the user could potentially hit NVDA+tab to report the focus if they wanted to double check (since that doesn't use the buffer). With the caching, they'll have to wait a second before this will work.

Firefox also doesn't fire a nameChange when descendant text changes:

data:text/html,<button onClick="this.textContent = 'Play';">Pause</button>

However, i think the caching will be okay with this one because it does fire a textInserted event and the cache does get invalidated for this.

Comment 9

[Eitan Isaacson [:eeejay]]

Comment on attachment 8895521 [details] [diff] [review]
Part 1: Add time-based expiry to handler cache

Review of attachment 8895521 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/ipc/win/handler/AccessibleHandler.cpp
@@ +28,5 @@
>  #include "Accessible2_i.c"
>  #include "Accessible2_2_i.c"
>  #include "Accessible2_3_i.c"
>  
> +static const ULONGLONG kCacheTtl = 10000000ULL; // 1 second in 100ns units

It would be interesting to have some tooling to know how many hits/misses we get. 1 second seems kind of arbitrary, and may be worth tuning.

@@ +124,2 @@
>    uint32_t gen = ctl->GetCacheGen();
> +  refresh |= gen != mCacheGen;

Unrelated to this change, but does gen ever change? I can't find where ctl Invalidate() is ever called.

Comment 10

[Eitan Isaacson [:eeejay]]

Comment on attachment 8895528 [details] [diff] [review]
Part 2: Cache additional fields

Review of attachment 8895528 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/ipc/win/HandlerProvider.cpp
@@ +141,5 @@
>    return S_OK;
>  }
>  
> +template <typename CondFnT, typename ExeFnT>
> +class MOZ_RAII ExecuteWhen final

I have never seen this kind of trick before. Cunning.

@@ +199,5 @@
> +  hr = target->accLocation(&aOutIA2Data->mLeft, &aOutIA2Data->mTop,
> +                           &aOutIA2Data->mWidth, &aOutIA2Data->mHeight,
> +                           kChildIdSelf);
> +  if (FAILED(hr)) {
> +    return;

Here and below, we can't recover after one failed getter and cache the rest?

@@ +205,5 @@
> +
> +  hr = target->get_accRole(kChildIdSelf, &varVal);
> +  if (FAILED(hr)) {
> +    return;
> +  }

Except for documents, I think accessibles are recreated on role change. So you may be able to have this persisted. But I guess this is a good first pass.

Right, Alex?

@@ +258,5 @@
> +  if (FAILED(hr)) {
> +    return;
> +  }
> +
> +  aOutIA2Data->mHwnd = PtrToLong(hwnd);

could the hwnd change? Is there a reason for it to be invalidated like the rest of these fields?

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 8895530 [details] [diff] [review]
Part 3: Add invalidation support to AccessibleWrap

Review of attachment 8895530 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/ipc/win/DocAccessibleChild.cpp
@@ +8,5 @@
>  
>  #include "nsAccessibilityService.h"
>  #include "Accessible-inl.h"
>  #include "mozilla/a11y/PlatformChild.h"
> +#include "mozilla/a11y/AccessibleWrap.h"

You include this here, but don't use it. Could this be removed?

::: accessible/windows/msaa/AccessibleWrap.cpp
@@ +1682,5 @@
> +
> +    ASYNC_INVOKER_FOR(IHandlerControl) invoker(controller.mCtrl,
> +                                               Some(controller.mIsProxy));
> +
> +    HRESULT hr = ASYNC_INVOKE(invoker, Invalidate);

Ah, here is where you call it. Cooool

Comment 12

[Eitan Isaacson [:eeejay]]

Comment on attachment 8895532 [details] [diff] [review]
Part 4: Force invalidation when certain events fire

Review of attachment 8895532 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/windows/msaa/AccessibleWrap.cpp
@@ +1188,5 @@
>      return;
>    }
>  
> +  if (IsHandlerInvalidationNeeded(winEvent)) {
> +    InvalidateHandlers();

This seems like a blunt caching scheme, and I can see the appeal of that.

The downside is that an accessible won't be cached at all if it is scrolled, for example, and the location keeps changing. An alternative would be to only invalidate the location field, and still cache the name field. If the location is requested in the client again, it would refresh the entire payload because I'm assuming it would be cheap.

I think we should probably remain cautious and aggressive with invalidations for now, but there is definitely room for tuning in the future. That's why I think it would be cool to have tooling for the cache (maybe A11Y_LOG printouts?)

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

I'm assuming we need this to ship?

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Yeah.

Comment 15

[James Teh [:Jamie]]

Comment on attachment 8895528 [details] [diff] [review]
Part 2: Cache additional fields

Review of attachment 8895528 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/ipc/win/HandlerProvider.cpp
@@ +207,5 @@
> +  if (FAILED(hr)) {
> +    return;
> +  }
> +
> +  aOutIA2Data->mRole = varVal.lVal;

accRole can return a VT_BSTR (e.g. for divs and blockquotes), so this case needs to be handled here.

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

NI Eitan since he mentioned re-reviewing and we want to wait for that.

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

Jamie I heard you have rebased patches, can you attach them here and kick off a try build for Marco?

Comment 18

[James Teh [:Jamie]]

Attachment: Part 3: Add invalidation support to AccessibleWrap

Tweaked to apply against current mozilla-central.

David, I don't have try access yet. I'll follow that up.

Comment 19

[James Teh [:Jamie]]

Eitan, wasn't sure how to (and whether I even should) carry your review forward. The change is very tiny; just some compat stuff changed in QueryInterface which broke the original patch.

Aaron, I guess mozreview will be out of sync with this, since I just manually attached the patch here?

Comment 20

[James Teh [:Jamie]]

Here's a try run with the latest patches:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66e29234b5fc9e814dde0915fc54a7aee5d715cf&selectedJob=135303635

Comment 21

[James Teh [:Jamie]]

Marco and I have both tried these patches and are seeing no noticeable perf improvement with NVDA... which is surprising. I haven't tested so much with JAWS yet, but I don't think I'm seeing anything there either. Some findings that might explain this:

1. NVDA vbuf uses IAccessibleHypertext::hyperlink to access embedded objects. In fact, the only time NVDA vbuf uses AccessibleChildren is if the text interface isn't supported (e.g. tables). This method returns an IAccessibleHyperlink, which we then QI to IAccessible2. Unfortunately, when we do this, it seems to bypass the handler altogether. The practical upshot is that NVDA only ever uses the cache on the top level document, which totally explains why the cache is showing no practical benefit at this point.
I guess the handler can't handle IAccessibleHyperlink, so it doesn't get marshaled as something that should use the handler.

2. The timer could be a problem for buffer renders if the client fetches some properties, recurses into the subtree and then fetches some more properties. I just checked and it doesn't seem NVDA ever does this, but I was seeing a lot of calls to IAccessible2::attributes from JAWS on the same node. I'd need to dig into this further to see whether this is actually what's happening, though.

3. JAWS calls IAccessible2_2::attribute (single attribute fetch) for quite a few attributes. The irony is that this was added to IA2 to improve performance, since the server doesn't have to generate *all* attributes if the client is only interested in one. However, for e10s, it makes things worse because each attribute fetched in this way is an extra cross-proc call, which probably far outweighs the perf penalty of fetching all attributes in content.
I think we might need to implement a handler version of this which parses the full cached attributes string and returns the single attribute requested.

Comment 22

[Eitan Isaacson [:eeejay]]

Comment on attachment 8915374 [details] [diff] [review]
Part 3: Add invalidation support to AccessibleWrap

Review of attachment 8915374 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly a rubber stamp from me. I wanted to see these patches in master as soon as possible, but it looks like we aren't getting the gains we want from them. Either way this is a good first step.

::: accessible/ipc/win/DocAccessibleChild.cpp
@@ +8,5 @@
>  
>  #include "nsAccessibilityService.h"
>  #include "Accessible-inl.h"
>  #include "mozilla/a11y/PlatformChild.h"
> +#include "mozilla/a11y/AccessibleWrap.h"

Does including this header do something? Is this where you wanted to call InvalidateHandlers?

::: accessible/windows/msaa/AccessibleWrap.cpp
@@ +1687,5 @@
> +
> +    if (hr == CO_E_OBJNOTCONNECTED || hr == kErrorServerDied) {
> +      sHandlerControllers->RemoveElement(controller);
> +    } else {
> +      NS_WARN_IF(FAILED(hr));

What other reasons are there for this to fail? Is it worth keeping the handlers or discarding them? This is probably fine.

::: accessible/windows/msaa/AccessibleWrap.h
@@ +198,5 @@
>    static void ReleaseContentProcessIdFor(dom::ContentParentId aIPCContentId);
>  
>    static void SetHandlerControl(DWORD aPid, RefPtr<IHandlerControl> aCtrl);
>  
> +  static void InvalidateHandlers();

Am I missing where this is actually used?

Comment 23

[Eitan Isaacson [:eeejay]]

(In reply to James Teh [:Jamie] from comment #21)
> Marco and I have both tried these patches and are seeing no noticeable perf
> improvement with NVDA... which is surprising. I haven't tested so much with
> JAWS yet, but I don't think I'm seeing anything there either. Some findings
> that might explain this:
> 
> 1. NVDA vbuf uses IAccessibleHypertext::hyperlink to access embedded
> objects. In fact, the only time NVDA vbuf uses AccessibleChildren is if the
> text interface isn't supported (e.g. tables). This method returns an
> IAccessibleHyperlink, which we then QI to IAccessible2. Unfortunately, when
> we do this, it seems to bypass the handler altogether. The practical upshot
> is that NVDA only ever uses the cache on the top level document, which
> totally explains why the cache is showing no practical benefit at this point.
> I guess the handler can't handle IAccessibleHyperlink, so it doesn't get
> marshaled as something that should use the handler.

So caching IAccessibleHypertext::hyperlink in the handler would be impossible?

Comment 24

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #21)

> 3. JAWS calls IAccessible2_2::attribute (single attribute fetch) for quite a
> few attributes. The irony is that this was added to IA2 to improve

IAccessible2_2::attribute is not yet implemented, see bug 740764, see also:

https://dxr.mozilla.org/mozilla-central/source/accessible/ipc/win/handler/AccessibleHandler.cpp#853
https://dxr.mozilla.org/mozilla-central/source/accessible/windows/ia2/ia2Accessible.cpp#497

Comment 25

[James Teh [:Jamie]]

Ah, thanks; that's good to know. While the handler does return E_NOTIMPL for extendedStates, it actually passes the call through for attribute; see line 991. We should just return E_NOTIMPL for now.

Comment 26

[James Teh [:Jamie]]

(In reply to Eitan Isaacson [:eeejay] from comment #23)
> So caching IAccessibleHypertext::hyperlink in the handler would be
> impossible?

There are two parts to this question:
1. Caching the response to IAHypertext::hyperlink as part of the request for the parent (and we'd need to cache all of them) would mean caching children, since hyperlink is actually just an interface of a child. We'd end up caching the entire tree. :)
2. What we really want to do is make sure the handler gets used for IAccessibleHyperlink so that when we get an instance of it and QI to IAccessible2, we benefit from the cache. The issue is that right now, the handler can't handle IAccessibleHyperlink; it doesn't know about it. The question is how we teach it, which is complicated somewhat by the fact that IAHyperlink is not available for *all* objects (e.g. table cells).

Comment 27

[James Teh [:Jamie]]

Comment on attachment 8895528 [details] [diff] [review]
Part 2: Cache additional fields

Review of attachment 8895528 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/ipc/win/handler/AccessibleHandler.cpp
@@ +755,4 @@
>    }
> +
> +  BEGIN_CACHE_ACCESS;
> +  GET_FIELD(mIA2Role, *role);

This isn't set by HandlerProvider::BuildIA2Data, so this breaks anything that has an IA2 role such as headings. Marco got bitten by this with my try build recently. :)

Comment 28

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1961b296e905dc373904c22e0aa478c0ebc8ecfa
Bug 1363595: Add additional fields to accessible handler cache; r=eeejay

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c77476105047f85aec1af2041e8f878c2ccf35
Bug 1363595: Add support for COM handler invalidation events to msaa AccessibleWrap; r=eeejay

https://hg.mozilla.org/integration/mozilla-inbound/rev/750ec8b9a88093fb4550e768d27183891acdf143
Bug 1363595: Invalidate accessible handler cache when change events occur; r=eeejay

Comment 29

[(No longer employed by Mozilla) Aaron Klotz]

As discussed with Jamie, I held off on landing part 1. We'll re-evaluate time-based invalidation at a later date.

(In reply to James Teh [:Jamie] from comment #27)
> This isn't set by HandlerProvider::BuildIA2Data, so this breaks anything
> that has an IA2 role such as headings. Marco got bitten by this with my try
> build recently. :)

I fixed that in the patch as landed.

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

Why does the whiteboard say DUPE ME and do we want to track this cache/perf meta as part of our aes+ triage?

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1961b296e905
https://hg.mozilla.org/mozilla-central/rev/f0c774761050
https://hg.mozilla.org/mozilla-central/rev/750ec8b9a880

Comment 32

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment #30)
> Why does the whiteboard say DUPE ME and do we want to track this cache/perf
> meta as part of our aes+ triage?

I have no idea why it has dupe me. Removing.

We can track for aes+ via the more general perf bugs.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Too late to uplift to 57 without a really compelling reason for doing so.