New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gecko VBufBackend: greatly speed up subtree re-renders when part of a document has changed #8678

Merged
merged 17 commits into from Sep 25, 2018

Conversation

Projects
None yet
7 participants
@michaelDCurran
Contributor

michaelDCurran commented Aug 27, 2018

Link to issue number:

None.

Summary of the issue:

Currently when our virtualBuffers are told that a node is invalid and needs re-rendering, the backend re-renders the entire subtree. This is true even if none of the node's children actually changed.
This wastes a lot of time as in many cases only a property on the node itself changed, but all its children remain unchanged.
This is especially noticeable in newer Firefox builds where renders now take somewhat longer.
The best example of this is on the page:
https://de.wikipedia.org/wiki/Liste_der_Tatort-Folgen
The page itself is extraordinarily large, however every time a link is focused, a tooltip (or something) is appended to the end of the document. This causes NVDA's virtualBuffer to re-render the entire document. Yet in reality the only thing that changed was a node got added at the end.
From the users' point of view, they notice a lag of up to 15 seconds every time they arrow onto or tab to a link.

Description of how this pull request fixes the issue:

VBufStorage buffers and backends have been slightly changed to allow a render in a temporary buffer to reuse existing nodes for one or more of its descendants if the existing node itself has not been marked as invalid.
Specific changes:

  • Much of the code in VBufStorage_buffer_t::removeFieldNode has been abstracted out into a new unlinkFieldNode method.
  • A new moveSubtree method has been added to VBufStorage_buffer_t.
  • A new VBufStorage_referenceNode_t class has been added. This class is a subclass of fieldNode and holds a secondary existing node. this class is used only when rendering into a temporary buffer. These reference nodes will be replaced with the existing node when the subtree is merged back into the original buffer.
  • A new addReferenceNodeToBuffer method has been added to VBufStorage_buffer_t which takes an existing node from another buffer, and adds it to this buffer as a reference node, at the location given by parent and previous. Like the other addXXToBuffer methods it returns the referenceNode that was added.
  • VBufBackend_t::invalidateSubtree has been changed to store invalid subtrees such that a node will always come after any of its ancestors. This ensures that when re-rendering, we always render parents before children.
  • VBufStorage_buffer_t::replaceSubtrees has been changed such that after all re-renders for all invalid subtrees has occurred, each temporary buffer's reference nodes if any are replaced with the actual nodes they point to, thereby sticking the original subtrees back into the newly rendered content, before it is then finally all put back into the original buffer.
  • A new reuseExistingNodeInRender method has been added to VbufBackend_t which looks up an existing node with a given docHandle and ID, and returns it if the existing node is not itself marked as invalid. If it is however marked as invalid, the existing node is unmarked as being invalid and this method does not return the node. This is so that a backend that is currently re-rendering an invalid node's children, it can re-render any of the children if they exist but are marked as invalid, and therefore need to be re-rendered anyway.
  • Gecko VBufBackend's fillVBuf method: If rendering a descendant of an invalid subtree, it tries to reuse existing nodes by calling reUseExistingNodeInRender. If one is returned, it adds it to the buffer as a reference node, otherwise it continues to render the node as normal.

Only the Gecko VBufBackend makes use of this new functionality. Therefore this affects Firefox and Chrome.
This was briefly tried for MSHTML but very strange results occurred when moving nodes with javascript.

Testing performed:

In Firefox and Chrome, Tested NVDA on the German wikipedia page. Plus NV Access's dynamic_content_tests, moveNodes and formFields html tests.

Known issues with pull request:

None so far. But does require a lot of user testing to ensure information doesn't go missing, or no crashes start occurring.

Change log entry:

Bug fixes:

  • Performance increases in Mozilla Firefox when navigating large pages with lots of dynamic changes.

michaelDCurran added some commits Aug 23, 2018

MSHTML vbufBackend: don't do partial re-renders as strange things hap…
…pen when moving nodes with javascript.

@michaelDCurran michaelDCurran requested a review from jcsteh Aug 27, 2018

@jcsteh

yay!

@@ -26,7 +26,7 @@ using namespace std;
VBufBackendSet_t VBufBackend_t::runningBackends;
VBufBackend_t::VBufBackend_t(int docHandleArg, int IDArg): renderThreadID(GetWindowThreadProcessId((HWND)UlongToHandle(docHandleArg),NULL)), rootDocHandle(docHandleArg), rootID(IDArg), lock(), renderThreadTimerID(0), invalidSubtreeList() {
VBufBackend_t::VBufBackend_t(int docHandleArg, int IDArg): renderThreadID(GetWindowThreadProcessId((HWND)UlongToHandle(docHandleArg),NULL)), rootDocHandle(docHandleArg), rootID(IDArg), lock(), renderThreadTimerID(0), pendingInvalidSubtreesList() {

This comment has been minimized.

@jcsteh

jcsteh Aug 27, 2018

Contributor

nit: We initialise pendingInvalidSubtreesList here, but not workingInvalidSubtreesList. It'd be good to be consistent here. I don't think they need to be initialised because they're C++ types and thus have a defined initial state.

break;
} else if(isDescendantNode(node,existingNode)) { //A descedndant has been invalidated
LOG_DEBUG(L"removing a descendant node from the invalid nodes");
//Remove the old descendant

This comment has been minimized.

@jcsteh

jcsteh Aug 27, 2018

Contributor

We no longer remove descendants in invalidateSubtree. That's fine for gecko_ia2 because while rendering a parent, reuseExistingNode will un-invalidate a child node if it isn't reused. If it's a grandchild or deeper, we do want update to re-render it anyway. However, this is problematic for other backends which don't use reuseExistingNode because they always re-render all descendants in fillVBuf, but those descendants will then get re-rendered again by update. Either:

  1. reuseExistingNode must be used by all backends. This means fixing the bugs with mshtml. As discussed, this is probably due to updateAncestor.
  2. This new behaviour of invalidateSubtree where it keeps descendants must only be used for gecko_ia2.

Option 1 is preferable, but failing that, option 2 will be necessary.

@@ -200,10 +186,11 @@ void VBufBackend_t::update() {
node->getIdentifier(&docHandle,&ID);
LOG_DEBUG(L"subtree node has docHandle "<<docHandle<<L" and ID "<<ID);
LOG_DEBUG(L"Rendering content");
render(tempBuf,docHandle,ID,node);
render(tempBuf,docHandle,ID,node);

This comment has been minimized.

@jcsteh

jcsteh Aug 27, 2018

Contributor

nit: A pesky trailing tab got added here.

// This existing node was not marked as invalid, therefore it can be re-used.
return existingNode;
} else {
// this existing node was marked as invalid also, therefore we can re-render it here anyway.

This comment has been minimized.

@jcsteh

jcsteh Aug 27, 2018

Contributor

This comment is a bit confusing. Perhaps: "This existing node was marked as invalid, so the caller must now re-render it."

// Node not known
return;
}
if(i->second==node) {

This comment has been minimized.

@jcsteh

jcsteh Aug 27, 2018

Contributor

These two asserts have been converted into if checks. Is there any reason these cases should now be happening in normal operation where they didn't before? If not, this change should be removed.

@@ -629,6 +629,14 @@ bool VBufStorage_buffer_t::replaceSubtrees(map<VBufStorage_fieldNode_t*,VBufStor
m.erase(i++);
continue;
}
for(auto i=buffer->referenceNodes.rbegin();i!=buffer->referenceNodes.rend();++i) {

This comment has been minimized.

@jcsteh

jcsteh Aug 27, 2018

Contributor

Can we please have a comment explaining why this is iterated in reverse?

@@ -1159,6 +1176,28 @@ bool VBufStorage_buffer_t::isNodeInBuffer(VBufStorage_fieldNode_t* node) {
return this->nodes.count(node)?true:false;
}
bool VBufStorage_buffer_t::moveSubtree(VBufStorage_fieldNode_t* node, VBufStorage_controlFieldNode_t* newParent, VBufStorage_fieldNode_t* newPrevious) {

This comment has been minimized.

@jcsteh

jcsteh Aug 27, 2018

Contributor

This method isn't used by these changes and should be removed (or used in replaceSubtrees).

/**
* uniquely identifies this control in its buffer.
*/
const VBufStorage_controlFieldNodeIdentifier_t identifier;

This comment has been minimized.

@jcsteh

jcsteh Aug 27, 2018

Contributor

Any reason this is now public where it was previously protected?

/**
* Unlinks this node from its parent and siblings.
* Note that this does not delete not enode, nore unregister it from the buffer in any other way.

This comment has been minimized.

@jcsteh

jcsteh Aug 27, 2018

Contributor

typo: "not enode" -> "node"?

/**
* Unlinks this node from its parent and siblings.
* Note that this does not delete not enode, nore unregister it from the buffer in any other way.
* This is used internally by removeFieldNode and moveSubtree.

This comment has been minimized.

@jcsteh

jcsteh Aug 27, 2018

Contributor

If you do remove moveSubtree, be sure to update this comment.

@Brian1Gaff

This comment has been minimized.

Brian1Gaff commented Aug 27, 2018

@MarcoZehe

This comment has been minimized.

Contributor

MarcoZehe commented Aug 27, 2018

@jcsteh gave me this try build to test, and I found a possible regression. Under some circumstances, links become duplicated, and this is not reproducible in a current Alpha build. The only way I have so far been able to reproduce this is as follows:

  1. If you don't have one, create an account in the federated Mastodon network.
    • Open an account at a11y.info, for example. It's fully accessible.
  2. Then, visit the alternative web client Pinafore.
    • Activate the Add Instance link.
    • Type a11y.info in the Instance field and press Enter.
    • Authorize Pinafore to use your account you created in step 1.
  3. Once the instance is added, you're being brought to the Home tab. Now, find the Settings link and activate it.

Result: The General link appears once, the Instances and About links appear twice.

Likewise if you activate the Instances link, after the updated UI, the instance name and Add Another instance links appear twice as well.

@DrSooom

This comment has been minimized.

DrSooom commented Aug 28, 2018

The Bugzilla-ID is Bug 1473310. (I'm that guy from Austria.)

Here are the log files with NVDA 2018.1 (installed) and with try-15940 (portable). Firefox 61.0.2 was running on the same USB 3.0 flash drive as the NVDA try build. In NVDA 2018.1 the cursor jumped suddenly around even no key was pressed and in try 15940 the "mouse hover" information disappear "immediately" after the cursor left of the link and was moved to the bottom of the document (Liste der Tatort-Folgen). The loading time when jumping from link to link was quite reduced in the try build in compare with NVDA 2018.1 as well as the huge lagging situation after switching back to Firefox. As Firefox 61.0.2 was running from an USB flash drive, I don't want to compare the initialling loading time (= first load of this wiki article) with Firefox 56.0.2 which is still installed on a SSD.

System: Win7x64 on a SSD, Intel Core i7-960, 6 GB RAM.

btw: Next time I must copy my "gestures.ini" into the portable copies of NVDA. It is really horrible for me to use NVDA without this. ;)

@jcsteh

This comment has been minimized.

Contributor

jcsteh commented Aug 28, 2018

@DrSooom

This comment has been minimized.

DrSooom commented Aug 29, 2018

In try-15940 (portable version) I noticed that the whole update check checkboxes (including that one for the additional stats) disappeared in the General NVDA Settings. furthermore the menu item "Check for updates..." (or like this) in the NVDA Help menu has also been gone in the portable version of this build. This issue doesn't appeared in 2018.3 Beta 3. So I think it's better to ask here about these new strange things than opening a new issue especially for it. alpha-15933 wasn't tested.

@jcsteh

This comment has been minimized.

Contributor

jcsteh commented Aug 29, 2018

@MarcoZehe

This comment has been minimized.

Contributor

MarcoZehe commented Sep 5, 2018

After having run with this try build for days, except for the doubled links issue in that one instance I described above, and a table update issue I discussed with @jcsteh, I have not found any other problems with it. My current illness having reduced my computer time significantly not withstanding. But I would give this a thumbs up.

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Sep 5, 2018

@MarcoZehe

This comment has been minimized.

Contributor

MarcoZehe commented Sep 5, 2018

I think we would be best no longer filling in row and column numbers /
spans / headers in the vbufBackend and instead find them at runtime in
NVDA's process.

OK, then it would probably be best if we tried caching these in the handler in Firefox in the future, or?

@jcsteh

This comment has been minimized.

Contributor

jcsteh commented Sep 5, 2018

michaelDCurran added some commits Sep 6, 2018

VBufStorage and backends: If a part of a table needs re-rendering, ma…
…ke sure to rerender any part of the table after that. E.g. If a row is added, then all rows after that also need to be re-rendered.
VBufStorage and backends: ensure that a node and its descendants are …
…all re-rendered if a node has been given alternative text because the node had no useful descendants (E.g. an empty link).
VBufStorage and backends: remove some log calls, beeps, and also swit…
…ch gecko_ia2 virtualBuffer from AccessibleObjectFromEvent to accChild for getNVDAObjectFromIdentifier.
@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Sep 7, 2018

Okay. Another try build:
https://ci.appveyor.com/api/buildjobs/fpjh24sbuu9jfv78/artifacts/output%2Fnvda_snapshot_try-vbufPartialRender-15991%2C9a516e70.exe

I spent some more time on this so that if part of a table changes, all parts of the table following the change are also re-rendered, keeping row and column coordinates accurate.

This again has the German Wikipedia page not blocking on updates.

I have also managed to fix the pinafore double link problem. I actually have a feeling that Gecko is not firing a needed winEvent on the span inside the link when it gains a textNode... though I can't be completely sure.

A lot more review required I'm sure, as another 2 properties have been added to VbufStorage_controlFieldNode_t.
However, I did manage to address all @jcsteh's previous comments.

@MarcoZehe

This comment has been minimized.

Contributor

MarcoZehe commented Sep 8, 2018

Hi @michaelDCurran, you wrote:

I have also managed to fix the pinafore double link problem. I actually have a feeling that Gecko is not firing a needed winEvent on the span inside the link when it gains a textNode... though I can't be completely sure.

I still see this in the latest Try build. The Pinafore Instances, About links in Settings, and once you open Instances, the Instance name links are still being duplicated in the buffer. This is with Firefox (now 64) Nightly, but also 62 release and 63 beta.

michaelDCurran added some commits Sep 10, 2018

VBufStorage and backends: Ensure that reuse is denied when previous s…
…iblings have changed, only if a node's denyReuseIfPreviousSiblingsChanged property is actually set (woops). Also, remove ensureDescendantsRequireParentUpdate method and replace it with a alwaysRerenderDescendants property instead.

@michaelDCurran michaelDCurran force-pushed the vbufPartialRender branch from 7a45337 to e42f93b Sep 10, 2018

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Sep 10, 2018

@MarcoZehe

This comment has been minimized.

Contributor

MarcoZehe commented Sep 10, 2018

On my system that link is certainly not being duplicated now.

I am happy to report that the latest try build fixes the duplicate links for me as well, thanks @michaelDCurran! :)

LOG_DEBUG(L"Setting node's requiresParentUpdate to true");
parentNode->requiresParentUpdate=true;
LOG_DEBUG(L"Setting node's alwaysRerenderChildren to true");
parentNode->alwaysRerenderChildren=true;

This comment has been minimized.

@jcsteh

jcsteh Sep 20, 2018

Contributor

It'd be good to have some brief comments here explaining why we set these things. For example: if a row is added or removed, we must re-render any rows after it so we get updated coordinates. If a row changes, we must re-render its cells so we get updated coordinates.

LOG_DEBUG(L"Setting node's denyReuseIfPreviousSiblingsChanged to true");
parentNode->denyReuseIfPreviousSiblingsChanged=true;
LOG_DEBUG(L"Setting node's requiresParentUpdate to true");
parentNode->requiresParentUpdate=true;

This comment has been minimized.

@jcsteh

jcsteh Sep 20, 2018

Contributor

I'm pretty sure we talked about this, but now i can't remember. Why does a change to a row require updating the table itself (its parent)? A change to an ARIA rowgroup might, but even that would be satisfied by updateTableCounts. Or does this avoid the need for updateTableCounts altogether? Should that be removed?

this->requestUpdate();
return true;
}
void VBufBackend_t::update() {
if(inUpdate) {
return;

This comment has been minimized.

@jcsteh

jcsteh Sep 20, 2018

Contributor

Is this to prevent updates requested by win events during a render from running within COM calls? It might be worth commenting the reason for possible re-entry. Also, if this does occur, how does the pending update get re-requested? I guess some other event will fire eventually, but that seems unreliable. Or am I missing something?

}
VBufStorage_referenceNode_t* previousReferenceNode=dynamic_cast<VBufStorage_referenceNode_t*>(previousControlFieldNode);
if(previousControlFieldNode&&!previousReferenceNode) {
LOG_DEBUG(L"Previous controlFieldNode was not a referenceNode");

This comment has been minimized.

@jcsteh

jcsteh Sep 20, 2018

Contributor

A comment here would be nice explaining that if the previous ControlFieldNode isn't a reference node, that means it is a newly added node.

Show resolved Hide resolved nvdaHelper/vbufBase/backend.cpp Outdated
*/
bool alwaysRerenderChildren {false};
/*

This comment has been minimized.

@jcsteh

jcsteh Sep 20, 2018

Contributor

Should be /** (double asterisk)

def setFocus(self):
oldFocus=self._getPhysicalFocus()
super(Mozilla,self).setFocus()
# Although all versions of Firefox block in setFocus until the physical focus has moved,

This comment has been minimized.

@jcsteh

jcsteh Sep 20, 2018

Contributor

This isn't quite correct. Firefox 57+ setFocus may return before the physical focus has moved. However, a subsequent call to accFocus will definitely block until the physical focus has moved. Same result, so no code change needed, but probably worth clarifying the comment.

michaelDCurran added some commits Sep 20, 2018

Address review comments:
* Add suggested comments.
* Gecko vbufBackend: move the setting of rerender control properties for parts of tables down to where the rest of the table code is, which means that the resetting of some of them on tabel cells is no longer necessary. The code remains at same scope, and  the function never returned or recursed between where the code was to where it is now, so this is safe.
* Gecko vbufBackend: it is no longer necesary to update table counts if a cell is rerendered. If the cell was being rerendered because it was specifically invalidated, then the row/column counts would not have changed, and if the cell was re-rendered due to an ancestor row / row group being invalidated, then the table node would have been rerendered anyway due to requiresParentUpdate being true on the row or row group. However, we still need the tableID from the table to set as an attribute on table cell nodes. Therefore, rename and rewrite updateTableCounts to getTableIDFromCell.
* VBufBackend_t::update: remove  the inUpdate check. This issue never actually seems to occur, and no code changes in this pr make it any more likely than it used to be.
Mozilla NVDAObject's _getPhysicalFocus method: handle case where accP…
…arent is None. Probably happens when an IAccessible is removed from the tree after we have fetched it... Happens quite a bit on facebook.com/
@MarcoZehe

This comment has been minimized.

Contributor

MarcoZehe commented Sep 20, 2018

Hi @michaelDCurran, is there a new try build by chance?

@MarcoZehe

This comment has been minimized.

Contributor

MarcoZehe commented Sep 21, 2018

I just earlier encountered a freeze in the Skype 8 Electron app while navigating back via landmarks. The freeze required me to kill NVDA and also the Skype app. Not 100% reproducible. The log from the start of me hitting Shift+D is:

Input: kb(laptop):shift+d
DEBUGWARNING - watchdog.watcher (10:04:31.637):
Trying to recover from freeze, core stack:
File "nvda.pyw", line 217, in
File "core.pyc", line 515, in main
File "wx\core.pyc", line 2134, in MainLoop
File "gui_init
.pyc", line 963, in Notify
File "core.pyc", line 486, in run
File "queueHandler.pyc", line 86, in pumpAll
File "queueHandler.pyc", line 53, in flushQueue
File "scriptHandler.pyc", line 145, in _queueScriptCallback
File "scriptHandler.pyc", line 187, in executeScript
File "browseMode.pyc", line 415, in
File "browseMode.pyc", line 379, in quickNavScript
File "virtualBuffers_init
.pyc", line 570, in _iterNodesByAttribs

WARNING - watchdog.watcher (10:04:46.677):
Core frozen in stack:
File "nvda.pyw", line 217, in
File "core.pyc", line 515, in main
File "wx\core.pyc", line 2134, in MainLoop
File "gui_init
.pyc", line 963, in Notify
File "core.pyc", line 486, in run
File "queueHandler.pyc", line 86, in pumpAll
File "queueHandler.pyc", line 53, in flushQueue
File "scriptHandler.pyc", line 145, in _queueScriptCallback
File "scriptHandler.pyc", line 187, in executeScript
File "browseMode.pyc", line 415, in
File "browseMode.pyc", line 379, in quickNavScript
File "virtualBuffers_init
.pyc", line 570, in _iterNodesByAttribs

// Set some properties to ensure that this and other nodes in the table aare correctly re-rendered if the table changes,
// so that the table's row and column cordinates remain accurate.
// setting denyReuseIfPreviousSiblingsChange ensures that if any part of the table is added or removed previous to this node,
// his node will not be reused (as its row / column coordinates would now be out of date).

This comment has been minimized.

@jcsteh

jcsteh Sep 24, 2018

Contributor

typo: his -> this

// setting requiresParentUpdate ensures that if this node is specifically invalidated,
// its parent will also be invalidated.
// For example, if this is a table row, its rerendering may change the number of cells inside.
// this in tern would affect the coordinates of all table cells in table rows after this row.

This comment has been minimized.

@jcsteh

jcsteh Sep 24, 2018

Contributor

typo: tern -> turn

if(!paccTableCell) { // just rows and row groups
// setting requiresParentUpdate ensures that if this node is specifically invalidated,
// its parent will also be invalidated.
// For example, if this is a table row, its rerendering may change the number of cells inside.

This comment has been minimized.

@jcsteh

jcsteh Sep 24, 2018

Contributor

Re-rendering a table row wouldn't change subsequent table row coordinates. However, this does apply to row groups, so you can just change the comment here (and on the next line) to reflect that.

@@ -297,6 +298,9 @@ VBufStorage_controlFieldNode_t* VBufBackend_t::reuseExistingNodeInRender(VBufSto
if(previousExistingControlFieldNode) break;
}
if(previousControlFieldNode!=previousExistingControlFieldNode) {
// The previous node differs from the existing previous node.
// We already know its not because a node was added, therefore this must be either a removal or a move.

This comment has been minimized.

@jcsteh

jcsteh Sep 24, 2018

Contributor

its -> it's

@michaelDCurran michaelDCurran merged commit 8377350 into master Sep 25, 2018

1 check was pending

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

@nvaccessAuto nvaccessAuto added this to the 2018.4 milestone Sep 25, 2018

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Sep 25, 2018

@PratikP1

This comment has been minimized.

PratikP1 commented Sep 26, 2018

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Sep 26, 2018

@PratikP1

This comment has been minimized.

PratikP1 commented Sep 26, 2018

My apologies. I forgot that sending attachments in plane text via email just pastes them inline rather than linked files.

@PratikP1

This comment has been minimized.

PratikP1 commented Sep 26, 2018

Updating to the latest alpha does solve my issues. Thank you. I'm no longer receiving the errors.

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Oct 26, 2018

Due to Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1502260
This PR makes it possible that some changes to table rows will not be reflected in browse mode.
Therefore:
It is quite likely this pr will be backed out of NVDA before 2018.4, and remerged after a fix is in a stable version of Firefox.
Note that before this pr is reverted, pr #8833 must first be reverted.

@MarcoZehe

This comment has been minimized.

Contributor

MarcoZehe commented Oct 26, 2018

@michaelDCurran Should this become necessary, will it only be backed out from the beta and release branches, or alpha as well? Because it would be good to keep it in Alpha I think, so we can continue testing possible fixes with this code.

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment