-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
Speed ups to ms word object model #9217
Conversation
…r only one layout column, remove some debugging code, and turn off screenUpdating while collecting text formatting.
… more efficiently, similarly to how we fetch comments and spelling errors.
} | ||
long start=0; | ||
if(_com_dispatch_raw_propget(pDispatchRevisionScope,wdDISPID_RANGE_START,VT_I4,&start)!=S_OK) { | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure, don't we need to log anything when one of these continue statements is reached, e.g. when we can fetch the type and start, but not the end? That might indicate that something is broken
@@ -945,7 +963,8 @@ void detectAndGenerateColumnFormatXML(IDispatchPtr pDispatchRange, wostringstrea | |||
} | |||
xmlStream <<L"text-column-number=\"" << columnNumber << "\" "; | |||
|
|||
// Finally, double check that we calculated the full width of the document | |||
/* | |||
The following commented out code to the end of this function can be used to double check that we calculated the full width of the document |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than commenting this out, would it make sense to add an additional nvdaHelperDebugFlag? May be you've already thought about this and thought it wouldn't be worth it. That's ok, I just wanted to bring up the idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, please remove / comment out / put behind a flag, the debug logging on line 920. I seem to recall that the logging from C++ took quite a long time.
LOG_DEBUG(L"ItemNumber: " << itemNumber
<< " rangePos: " << rangePos
<< " colStartPos: " << colStartPos);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOG_DEBUG is defined to nothing by default, I.e. none of those tokens within the brackets will be compiled, unless nvdahelperLogLevel is debug.
long revStart=get<0>(i); | ||
long revEnd=get<1>(i); | ||
long revType=get<2>(i); | ||
if(!(chunkStartOffset>=revEnd||chunkEndOffset<=revStart)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I somehow find this hard to read. I assume this is the same as:
if(!(chunkStartOffset>=revEnd||chunkEndOffset<=revStart)) { | |
if(chunkStartOffset<revEnd&&chunkEndOffset>revStart) { |
I still prefer a look from @feerrenrut, i'm getting better at C++ but COM interaction is still a bit tricky. I will also do some performance tests. |
Based on #5750 (comment), here is some performance data regarding table navigation,. str:
Below are results for the time between keypress and speech
This was with braille enabled. These tests are not very scientific, yet I'd have expected more clear differences. I believe you when you're saying that this improves things, but may be we should be careful about statements like twice faster, as for some situations, this might not apply |
Thanks for the tests.
I am very concerned that in many cases this pr is giving slower, not
faster, results. The code is definitely making less COM calls, and I am
certainly seeing major speed ups for long lines. Clearly though it has
become slower in tables? Very strange.
Are you absolutely sure that you have exactly the same NVDA config for
both, and that nvdaHelper was compiled the same way for both?
And of course, you double checked that NVDA was not using UIA for either
test? I.e. the role for the document was edit, not document?
I totally agree that we should be careful with the claims we make re speed.
|
The tests with master were performed with latest Alpha, whereas this pr's tested were performed from source. I will mitigate this difference by using a try build for a subsequent test in which I will take the average of at least three equivalent table movement commands.
Yes. |
Here is a new test, based om the average of 3 rounds instead of just one
This is with braille off, and certainly shows that the code that deals with revisions has been improved. Both builds were installed on my system. |
@@ -945,7 +963,8 @@ void detectAndGenerateColumnFormatXML(IDispatchPtr pDispatchRange, wostringstrea | |||
} | |||
xmlStream <<L"text-column-number=\"" << columnNumber << "\" "; | |||
|
|||
// Finally, double check that we calculated the full width of the document | |||
/* | |||
The following commented out code to the end of this function can be used to double check that we calculated the full width of the document |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, please remove / comment out / put behind a flag, the debug logging on line 920. I seem to recall that the logging from C++ took quite a long time.
LOG_DEBUG(L"ItemNumber: " << itemNumber
<< " rangePos: " << rangePos
<< " colStartPos: " << colStartPos);
nvdaHelper/remote/winword.cpp
Outdated
if(_com_dispatch_raw_propget(pDispatchWindow,wdDISPID_WINDOW_SELECTION,VT_DISPATCH,&pDispatchSelection)!=S_OK||!pDispatchSelection) { | ||
LOG_DEBUGWARNING(L"application.selection failed"); | ||
return; | ||
} | ||
//Disable screen updating as we will be moving the selection temporarily | ||
_com_dispatch_raw_propput(pDispatchApplication,wdDISPID_APPLICATION_SCREENUPDATING,VT_BOOL,false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems error prone, any early exit or future refactor could mean that screen updating is not turned back on.
I think it's worth trying to get to the bottom of the differences in performance improvement.
|
This pr creates duplicate branches speedUpMSWordObjectModel and speedupMSWordObjectModel differing only by capital U and causing a new branch to appear with each git pull. Can one of those be deleted? |
I removed the one with the lower case u from the server. You may need to
remove any local copies.
|
@LeonarddeR: Thanks again for these test results.
I have not specifically tested with that document you referenced, but I
think it is pointing to a slightly different issue which this pr alone
wouldn't really fix. that is, that when we move between table cells, we
try and speak the entire content of the cell. If the cell is very large
(I.e. more than a couple of paragraphs), this would take a very long
time for nvdaHelper to collect the content.
I would recommend that we change the general functionality for table
movement to only speak the first paragraph for a given cell. What are
your thoughts on this?
Have you done any testing with the test document.docx referenced by this
pr? I a especially interested in knowing if you see speed ups for most
lines in general. I certainly see a major speed up in the table of
contents, and in the two further sections that talk about font changes.
|
…pecifically from within each in-process call, disable it from Python the first time it is required in a core cycle, re-enabling it at the end of the core cycle.
@LeonarddeR: I cannot at all reproduce the massive slowdown in the document you used for the tests. Even with pages/changes/spelling enabled, I'm getting numbers like 0.23, not at all anything like 3.5 or higher. I'm not sure what language this is in, perhaps it is because I don't have a dictionary installed for that language, therefore spelling is really fast? |
Ah, it is Dutch :) Question is how to force MS Office to get a dictionary. |
With the Dutch language pack installed, I can reproduce it now. |
…fetch costly formatting info such as spelling errors and editor revisions, even if the user has these turned on.
Moving left and right in the test table is now down to 0.23 on my system. When speaking for unit paragraph or cell, we no longer fetch spelling errors or editor revisions as these can be very costly. This info will still automatically be spoken for character, word and line of course. |
@michaelDCurran commented on 3 Feb 2019, 23:04 CET:
I think this is a very good idea, as long as we somehow document this change for users and make it consistent for all edit controls.
Not yet. I will do this later today |
@michaelDCurran commented on 4 Feb 2019, 01:45 CET:
This is not Dutch, it is fake Latin, see lipsum.com. But I definitely had Dutch spell checking on when testing.
This means that table navigation does not report any spelling errors any more. Sometimes, table navigation is the only way one can effectively move through a table, and especially if the table has small cells, I'm not sure whether such a change would be appreciated. I wonder whether we could do anything with regard to the "Report formatting changes after the cursor (can cause a lag)" option in document formatting settings. It might make sense to implement this for Word |
We could use that config option, but who knows how people have that
configured now days. So, it may be very confusing for some.
Perhaps we should instead go with the other option I proposed which was
to only announce the first line of any given cell when table navigating?
Though with your worry above, perhaps even that might cause some
problems with navigating tables with small cells?
We need to really choose one of these two options.
|
Note also in that document, each cell only contains one paragraph.
Therefore even changing NVDA to fetch just the first paragraph does not
change anything. It must be just the first line. Also, Note that
pressing control+down / control+up (move by paragraph) is just as slow
as moving by cell currently. Thus why the change to speech cutting out
spelling errors worked well.
There is no way we can make it faster than this to fetch all spelling
errors for these extremely large paragraphs. We can only stop doing this
all together.
|
I'd rather go with the first line for table navigation, rather than the whole paragraph without errors. That's just my personal preference though.
|
…, don't fetch costly formatting info such as spelling errors and editor revisions, even if the user has these turned on." This reverts commit 6ea9d0f.
…f the next cell, otherwise it may take a very long time to collect all content for the cell.
source/NVDAObjects/window/winword.py
Outdated
@@ -343,6 +343,30 @@ | |||
u'\uF0FC' : u'\u2714', # Wingdings | |||
} | |||
|
|||
class ScreenUpdatingDisabler(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand the idea behind this, it doesn't look very pythonic to me. Is there any chance this can be implemented like a context manager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, as there isn't actually a common point at which we could use a with block to control the lifetime of the context manager. Its lifetime affects anything between the time it was created and the end of the core cycle. I guess we could possibly override caretMovementScriptHelper and wrap the super call in a with statement. But there may be other situations other than caret movement where this could be used. Right now it is placed above each nvdahelper call, which ensures that if it is not disabled already, it will be now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that in that case, the current solution might be best. I think we must make sure that it doesn't cause any visual issues, though, since screen updating might now be off longer than before.
It might still make sense to no longer report spelling errors and revision info when navigating by paragraph, regardless whether table navigation reports paragraphs or only the first line. I'd say that the paragraph movement commands are only used for quick scanning or reading, not within the process of authoring in which spelling errors and revisions are important. |
…ll not be announced. It is important that navigating by paragraph is performant as it is used for quick skimming.
# Therefore, only speak the first line of the cell. | ||
newInfo.collapse() | ||
newInfo.expand(textInfos.UNIT_LINE) | ||
speech.speakTextInfo(newInfo,reason=controlTypes.REASON_CARET, unit=textInfos.UNIT_LINE) | ||
newInfo.collapse() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now collapses twice, which means two extra com calls, one to collapse the range and one to get the new ends offset. Could we do anything about this, i.e. update the caret after the first collapse and than expand to line?
Also, may be you could optimize collapse while at it. It now fetches self._rangeObj.end in collapse without doing anything with it if end is not True. Something like this:
- newEndOffset = self._rangeObj.end
# the new endOffset should not have become smaller than the old endOffset, this could cause an infinite loop in
# a case where you called move end then collapse until the size of the range is no longer being reduced.
# For an example of this see sayAll (specifically readTextHelper_generator in sayAllHandler.py)
- if end and newEndOffset < oldEndOffset :
+ if end and self._rangeObj.end < oldEndOffset :
source/NVDAObjects/window/winword.py
Outdated
@@ -1150,6 +1180,13 @@ def script_previousColumn(self,gesture): | |||
|
|||
class WordDocument(Window): | |||
|
|||
def _get_screenUpdatingDisabler(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be explicitly state that this should be cached?
def _get_screenUpdatingDisabler(self): | |
_cache_screenUpdatingDisabler = True | |
def _get_screenUpdatingDisabler(self): |
nvdaHelper/remote/winword.cpp
Outdated
@@ -351,6 +328,45 @@ bool collectCommentOffsets(IDispatchPtr pDispatchRange, vector<pair<long,long>>& | |||
return !commentVector.empty(); | |||
} | |||
|
|||
bool collectRevisionOffsets(IDispatchPtr pDispatchRange, vector<tuple<long,long,long>>& revisionsVector) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spelling error equivalent of this function fetches the application object and checks whether it is sandboxed. Just to make sure, might this be a requirement for this new implementation? On the contrary, can we somehow avoid this sandbox check for spelling errors in some versions of word?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sandbox check was added for spelling errors before it was refacted into that function. I.e. MS Word would crash if range.spellingErrors.cound was ever called, no matter how big the range. I never found this to be the case for revisions. We could possibly version check where we need to look at sandbox, but I think the bug existed for 2010 and higher, so it would only slightly improve 2007. Still, we could revisit this again in future with a bit more testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be two space characters at the start of this line.
nvdaHelper/remote/winword.cpp
Outdated
@@ -983,8 +1005,13 @@ void winword_getTextInRange_helper(HWND hwnd, winword_getTextInRange_args* args) | |||
LOG_DEBUGWARNING(L"AccessibleObjectFromWindow failed"); | |||
return; | |||
} | |||
//Get the current selection | |||
IDispatchPtr pDispatchSelection=NULL; | |||
IDispatchPtr pDispatchApplication=NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this?
Regarding table navigation, just a thought from my side: I agree with stopping to report track changes and spelling errors while navigating by paragraph. Most people use browse mode or elements list to navigate between them anyway. |
While this is true, this also applies to the major performance issue caused by reading the whole cell instead of only the first line. Having said that, changing this behaviour for every application only because the Word performance is bad sounds a bit too drastic. @Adriani90: What do you think about the other way to improve this, reading the whole cell when navigating tables, thereby not speaking spelling and revisions? |
|
I noticed also a Performance issue especially when a table cell contains a list with bullets compared to a cell without bullets. I wonder if NVDA can handle the lists in MS Word in a different way, fetching the symbols maybe differently? |
… editor revisions and spelling errors (like when navigating by paragraph) are no longer spoken for navigating by table cell.
…. Editor revisions are okay.
…now, but do it from a RAAI class so that it can be re-enabled automatically at the end of scope.
Okay, so as to not make this pr too large for 2019.1, I have decided to not do the proposed changes for sections and text columns in this pr. We already have a significant speed-up in many scenarios. I have again switched back to announcing the entire table cell when navigating by cell, however, like with paragraphs, spelling errors are disabled for performance reasons. In the end I did not bother disabling editor revisions for cells or paragraphs, as this was not at all the cause of the performance hit in that document. |
I expect this pr to be merged quickly. I am use MS word mainly in company. I would like to experience the performance improvement through these results. |
// A class that disables MS Word screen updating while it is alive. | ||
class ScreenUpdatingDisabler { | ||
private: | ||
IDispatchPtr pDispatchApplication {nullptr}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not seen this kind of declaration earlier. What's the difference with this and IDispatchPtr pDispatchApplication = nullptr;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is new c++ syntax that allows you to provide the default value of a variable at declaration time. You can't use = for an instance variable in this context. It avoids having to initialize it in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions, happy for this to be merged when you are ready.
@@ -58,6 +58,35 @@ constexpr int formatConfig_initialFormatFlags =(formatConfig_reportPage|formatCo | |||
constexpr wchar_t PAGE_BREAK_VALUE = L'\x0c'; | |||
constexpr wchar_t COLUMN_BREAK_VALUE = L'\x0e'; | |||
|
|||
// A class that disables MS Word screen updating while it is alive. | |||
class ScreenUpdatingDisabler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
nvdaHelper/remote/winword.cpp
Outdated
@@ -351,6 +328,45 @@ bool collectCommentOffsets(IDispatchPtr pDispatchRange, vector<pair<long,long>>& | |||
return !commentVector.empty(); | |||
} | |||
|
|||
bool collectRevisionOffsets(IDispatchPtr pDispatchRange, vector<tuple<long,long,long>>& revisionsVector) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be two space characters at the start of this line.
nvdaHelper/remote/winword.cpp
Outdated
@@ -351,6 +359,45 @@ bool collectCommentOffsets(IDispatchPtr pDispatchRange, vector<pair<long,long>>& | |||
return !commentVector.empty(); | |||
} | |||
|
|||
bool collectRevisionOffsets(IDispatchPtr pDispatchRange, vector<tuple<long,long,long>>& revisionsVector) { | |||
IDispatchPtr pDispatchRevisions=NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind converting these to use nullptr
instead of NULL?
nvdaHelper/remote/winword.cpp
Outdated
@@ -351,6 +359,45 @@ bool collectCommentOffsets(IDispatchPtr pDispatchRange, vector<pair<long,long>>& | |||
return !commentVector.empty(); | |||
} | |||
|
|||
bool collectRevisionOffsets(IDispatchPtr pDispatchRange, vector<tuple<long,long,long>>& revisionsVector) { | |||
IDispatchPtr pDispatchRevisions=NULL; | |||
if(_com_dispatch_raw_propget(pDispatchRange,wdDISPID_RANGE_REVISIONS,VT_DISPATCH,&pDispatchRevisions)!=S_OK||!pDispatchRevisions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you convert these checks to get the result first, then check for success? They are currently so long, it makes it easy to miss other conditions for the branch.
if(_com_dispatch_raw_propget(pDispatchRange,wdDISPID_RANGE_REVISIONS,VT_DISPATCH,&pDispatchRevisions)!=S_OK||!pDispatchRevisions) { | |
auto res = _com_dispatch_raw_propget(pDispatchRange,wdDISPID_RANGE_REVISIONS,VT_DISPATCH,&pDispatchRevisions); | |
if(res!=S_OK || !pDispatchRevisions) { |
nvdaHelper/remote/winword.cpp
Outdated
// Disable screen updating until the end of this scope | ||
ScreenUpdatingDisabler sud{pDispatchApplication}; | ||
//Get the current selection | ||
IDispatchPtr pDispatchSelection=NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use nullptr
instead
What's New in 2019.1:
Interestingly, Word command Ctrl + Alt + N change to draft view. In NVDA side, this command starts or restarts NVDA. |
Hi Victor,
What i am suggesting, it’s better to change the NVDA keystroke to something different.
That’s what I’ve done in the Croatian translation, as it conflicts with the } sign.
What i suggest, it's the changing of the default gesture to something less conflict, for example ctrl+alt+f5.
|
@zstanecic, |
Link to issue number:
None.
Summary of the issue:
We introduced in-process support for Microsoft Word back in 2011 which drastically improved performance from the old out-of-process code. However, over the years as we have added even more features for MS Word, our in-process code has been getting slower and slower, and many users are starting to say that it is becoming impractical to use.
Description of how this pull request fixes the issue:
This pr provides between a 2x and 3x speed up in response time when navigating documents in Microsoft Word, when using the standard object model code. The UI Automation support is not touched by this pr.
This pr has been able to achieve the speed ups in the following ways:
Testing performed:
Note that the experimental UI Automation support for MS Word must be turned off to test this. PR 9200 is again making it disabled by default.
Using our standard
Test document.docx
I had NVDA read through the document, ensuring that revisions were still announced where expected. Plus sections.
I tested a separate document with multiple columns, to ensure that NVDA announced which column I was in.
On my Microsoft Surface Pro 4, plugged in with Power profile set to 100%, many lines have gone from taking 550 ms to now taking around 170 ms to start reading.
also I found it useful to set the view to draft mode for best performance -- something we should probably always recommend to users.
Known issues with pull request:
None.
Change log entry:
Bug fixes: