[e10s a11y] Expand a11y+e10s handler cache

RESOLVED FIXED in Firefox 58

Status

()

Core
Disability Access APIs
P1
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla58
Unspecified
Windows
hang, multiprocess, perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 disabled, firefox53 disabled, firefox54 disabled, firefox55 disabled, firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
No longer depends on: 1321960
Marking this as affecting 55 based on the original issue.
status-firefox53: --- → disabled
status-firefox54: --- → disabled
status-firefox55: --- → affected
(Assignee)

Updated

11 months ago
Blocks: 1365655
(Assignee)

Updated

11 months ago
Whiteboard: [dupe me], aes+ → [dupe me]
Do I need to track this? Is the title still accurate or do you want to change it?
status-firefox56: --- → affected
Flags: needinfo?(aklotz)
(Assignee)

Comment 3

10 months ago
No tracking yet, this bug is e10s only so we can mark it disabled for 55.
status-firefox55: affected → disabled
Flags: needinfo?(aklotz)
Summary: [e10s a11y] Browser becomes hangs up and Windows becomes almost unusable(all app becomes too slow. keyin slow).. → [e10s a11y] Expand a11y+e10s handler cache
(Assignee)

Comment 4

8 months ago
Created attachment 8895521 [details] [diff] [review]
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.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8895521 - Flags: review?(eitan)
(Assignee)

Comment 5

8 months ago
Created attachment 8895528 [details] [diff] [review]
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.
Attachment #8895528 - Flags: review?(eitan)
(Assignee)

Comment 6

8 months ago
Created attachment 8895530 [details] [diff] [review]
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.
Attachment #8895530 - Flags: review?(eitan)
(Assignee)

Comment 7

8 months ago
Created attachment 8895532 [details] [diff] [review]
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.
Attachment #8895532 - Flags: review?(eitan)

Comment 8

8 months ago
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 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.
Attachment #8895521 - Flags: review?(eitan) → review+
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?
Attachment #8895528 - Flags: review?(eitan) → review+
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
Attachment #8895530 - Flags: review?(eitan) → review+
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?)
Attachment #8895532 - Flags: review?(eitan) → review+

Updated

8 months ago
Blocks: 1372213
I'm assuming we need this to ship?
Flags: needinfo?(aklotz)
Priority: -- → P1
(Assignee)

Comment 14

7 months ago
Yeah.
Flags: needinfo?(aklotz)
status-firefox56: affected → wontfix
status-firefox57: --- → affected

Comment 15

7 months ago
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.
status-firefox57: affected → fix-optional
status-firefox58: --- → affected
NI Eitan since he mentioned re-reviewing and we want to wait for that.
Flags: needinfo?(eitan)
Jamie I heard you have rebased patches, can you attach them here and kick off a try build for Marco?
Flags: needinfo?(jteh)

Comment 18

7 months ago
Created attachment 8915374 [details] [diff] [review]
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.
Attachment #8895530 - Attachment is obsolete: true
Flags: needinfo?(jteh)
Attachment #8915374 - Flags: review?(eitan)

Comment 19

7 months ago
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 21

6 months ago
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 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?
Attachment #8915374 - Flags: review?(eitan) → review+
(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?
Flags: needinfo?(eitan)

Comment 24

6 months ago
(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

6 months ago
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

6 months ago
(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).

Updated

6 months ago
Blocks: 1409916

Updated

6 months ago
Blocks: 1409928

Comment 27

6 months ago
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. :)
(Assignee)

Comment 28

6 months ago
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
(Assignee)

Comment 29

6 months ago
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.
(Assignee)

Updated

6 months ago
Blocks: 1412103
Why does the whiteboard say DUPE ME and do we want to track this cache/perf meta as part of our aes+ triage?
https://hg.mozilla.org/mozilla-central/rev/1961b296e905
https://hg.mozilla.org/mozilla-central/rev/f0c774761050
https://hg.mozilla.org/mozilla-central/rev/750ec8b9a880
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Comment 32

6 months ago
(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.
Whiteboard: [dupe me]
Too late to uplift to 57 without a really compelling reason for doing so.
status-firefox57: fix-optional → wontfix
status-firefox-esr52: --- → disabled
You need to log in before you can comment on or make changes to this bug.