Bug 155376 - Poor performance in Calc UI when using Keyboard Viewer
Summary: Poor performance in Calc UI when using Keyboard Viewer
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
7.5.3.2 release
Hardware: x86 (IA32) macOS (All)
: medium normal
Assignee: Patrick Luby (volunteer)
URL:
Whiteboard: target:7.5.5 target:7.6.0 target:24.2...
Keywords:
Depends on:
Blocks: a11y-macOS
  Show dependency treegraph
 
Reported: 2023-05-17 19:56 UTC by bz137195
Modified: 2024-01-11 08:17 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description bz137195 2023-05-17 19:56:45 UTC
Description:
After Find and Replace… or Paste Special:Paste Special… there is a noticeable increase in the response time to mouse click, menu and command key events. The beach ball cursor appears frequently for short periods. It can take minutes to Quit LibreOffice.

Steps to Reproduce:
1. with Keyboard Viewer visible, launch LibreOffice
2. click Create:Calc Spreadsheet
3. click cell B2
4. type abc [return]
5. select menu Edit:Find and Replace…
6. replace b with t and click Replace All
7. click Close, click Close
8. repeat steps 5 to 7 with different replacements if needed

Actual Results:
Poor response to mouse and keyboard events. Need to force quit LibreOffice.

Expected Results:
Good response to mouse and keyboard events. Quit LibreOffice in timely fashion.


Reproducible: Always


User Profile Reset: No

Additional Info:
May be related to - https://bugs.documentfoundation.org/show_bug.cgi?id=153374

Version: 7.5.3.2 (X86_64) / LibreOffice Community
Build ID: 9f56dff12ba03b9acd7730a5a481eea045e468f3
CPU threads: 4; OS: Mac OS X 12.6.5; UI render: default; VCL: osx
Locale: en-GB (en_GB.UTF-8); UI: en-US
Calc: default
Comment 1 Patrick Luby (volunteer) 2023-05-18 00:04:52 UTC
Unfortunately, I am unable to reproduce this on my Mac Silicon machine.

What I did notice was that after repeating the steps for 10 times or so, when I closed the window and pressed the "Don't save" button, LibreOffice would hang for a few seconds. Despite my repeated attempts to get a sample, the hang would end before I could switch to the Activity Monitor applicaddtion and grab a sample.

Version: 7.5.3.2 (AARCH64) / LibreOffice Community
Build ID: 9f56dff12ba03b9acd7730a5a481eea045e468f3
CPU threads: 8; OS: Mac OS X 12.6.5; UI render: default; VCL: osx
Locale: en-US (en_US.UTF-8); UI: en-US
Calc: threaded
Comment 2 bz137195 2023-05-18 00:18:35 UTC
Thanks Patrick,
It seems to get worse if you do multiple repeat search/replace. And if you take more time doing it. Also try doing a few Paste Special:Paste Special…:Unformatted text operations.
Comment 3 Patrick Luby (volunteer) 2023-05-18 00:34:28 UTC
(In reply to bz137195 from comment #2)
> Thanks Patrick,
> It seems to get worse if you do multiple repeat search/replace. And if you
> take more time doing it. Also try doing a few Paste Special:Paste
> Special…:Unformatted text operations.

Thanks for the "paste special" tip. I now see a noticeable delay when using the arrow key to move to another cell after using your original steps a few times and then Paste Special::Unformatted text in a few random cells.

So when that happened, I opened Activity Monitor and saw that LibreOffice was using over a gigabyte of memory! That is just way too high for launching LibreOffice and opening a new spreadsheet.

My current theory is that you are probably right that my fix for issue 153374 is causing this bug. I suspect a memory leakage and the application gets slower and slower as more memory is allocated to LibreOffice. The memory leakage may be elsewhere in the LibreOffice code, but I will first see if I can modify my fix for issue 153374 without causing any memory leakage.
Comment 4 Commit Notification 2023-05-28 16:02:27 UTC
Patrick Luby committed a patch related to this issue.
It has been pushed to "libreoffice-7-5":

https://git.libreoffice.org/core/commit/37bcbfe633897820189d3c77bd69b7a796afbca4

Related tdf#tdf155376 fix minor memory leaks

It will be available in 7.5.5.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 5 Commit Notification 2023-05-28 23:10:15 UTC
Patrick Luby committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/cb4fb9e31e42ed719f435919d1ca671ba2d66863

Related tdf#tdf155376 fix minor memory leaks

It will be available in 7.6.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 6 Patrick Luby (volunteer) 2023-05-28 23:34:51 UTC
Unfortunately, I do not have a fix for this bug yet. The commits that I made today only fix a couple of small memory leaks in LibreOffice's macOS accessibility handling code that I found while looking for memory leaks.

The good news is that, after my commits today, I don't see any significant memory leakage after doing the steps that trigger this bug. So that eliminates my fix for issue #153374 as the cause.

I still see very high memory usage when native accessibility is enabled for LibreOffice (which the Keyboard Viewer as well as other tools do), but that memory gets reclaimed when all Calc documents are closed.

So back to square one for me.
Comment 7 Commit Notification 2023-06-07 13:56:02 UTC
Noel Grandin committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/b542d90447230e1d6705a665f3a3e524fa966b5d

tdf#155376 partially convert SvCTLOptions to officecfg

It will be available in 7.6.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 8 Commit Notification 2023-06-07 19:49:36 UTC
Patrick Luby committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/75dc3a54fca8b2dc775ba007190d8c2e188f1c0a

Partial fix tdf#155376 use NSAccessibilityElement instead of NSView

It will be available in 24.2.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 9 Patrick Luby (volunteer) 2023-06-08 14:37:49 UTC
So I have made some progress. I found that there are actually several different performance bottlenecks in both the macoS and Calc accessibility code.

Are you familiar with installing LibreOffice nightly builds? If yes, the June 8, 2023 or later nightly builds contain a fix for one the largest performance bottleneck in the macOS accessibility code:

https://dev-builds.libreoffice.org/daily/master/current.html

This should make arrowing around a Calc document much more responsive when the Keyboard Viewer or VoiceOver are enabled.

Note: you will still see a hang for several seconds when you close the Calc document. This is due to two performance bottlenecks that we have the following fix for, but the fix still needs to be reviewed before they will show up in a nightly build:

https://gerrit.libreoffice.org/c/core/+/152703
Comment 10 Patrick Luby (volunteer) 2023-06-08 14:43:52 UTC
Added a few more people to the cc list as I think this bug is really one case for a much larger bug: Calc hangs or crashes when accessibility is enabled.

I suspect that we are going to find several more hangs or crashes now that I have fixed this particular case.
Comment 11 Noel Grandin 2023-06-11 10:09:55 UTC
So one suggestion that I have is that in the below stack trace, (which is triggered when we do a replace-all operation),
we are creating tons of accessibility objects.

What I suggest, is that we instead return a custom implementation of NSArray, that lazily constructs the required underlying objects, instead of doing it eagerly.

That might be a lot more efficient, depending on what AppKit does with the returned object.

    frame #10: 0x00000001ab7d2a48 libsclo.dylib`ScAccessibleSpreadsheet::getAccessibleCellAt(this=0x0000000123e53f50, nRow=8, nColumn=3) at AccessibleSpreadsheet.cxx:943:56
    frame #11: 0x00000001ab7d41e6 libsclo.dylib`non-virtual thunk to ScAccessibleSpreadsheet::getAccessibleCellAt(int, int) at AccessibleSpreadsheet.cxx:0
  * frame #12: 0x000000011b2c0fbc libvclplug_osxlo.dylib`+[AquaA11yTableWrapper childrenAttributeForElement:](self=AquaA11yTableWrapper, _cmd="childrenAttributeForElement:", wrapper=0x00000001c0f312b0) at a11ytablewrapper.mm:87:92
    frame #13: 0x000000011b2c9015 libvclplug_osxlo.dylib`-[AquaA11yWrapper childrenAttribute](self=0x00000001c0f312b0, _cmd="childrenAttribute") at a11ywrapper.mm:336:16
    frame #14: 0x000000011b2cb172 libvclplug_osxlo.dylib`-[AquaA11yWrapper accessibilityAttributeValue:](self=0x00000001c0f312b0, _cmd="accessibilityAttributeValue:", attribute="AXChildren") at a11ywrapper.mm:698:25
    frame #15: 0x000000011b2d142b libvclplug_osxlo.dylib`-[AquaA11yWrapper accessibilityChildren](self=0x00000001c0f312b0, _cmd="accessibilityChildren") at a11ywrapper.mm:1335:12
    frame #16: 0x000000011b2d1461 libvclplug_osxlo.dylib`-[AquaA11yWrapper accessibilityChildrenInNavigationOrder](self=0x00000001c0f312b0, _cmd="accessibilityChildrenInNavigationOrder") at a11ywrapper.mm:1340:12
    frame #17: 0x00007fff2301b6b2 AppKit`-[NSAccessibilityAttributeAccessorInfo getAttributeValue:forObject:] + 58
Comment 12 Patrick Luby (volunteer) 2023-06-11 12:20:17 UTC
(In reply to Noel Grandin from comment #11)
> 
> What I suggest, is that we instead return a custom implementation of
> NSArray, that lazily constructs the required underlying objects, instead of
> doing it eagerly.
>

Still don't like my patch to SfxBroadcaster and ConfigurationBroadcaster? :D

Seriously though, your proposed solution tries to shift the responsibility for fixing a performance bottleneck in the core LibreOffice C++ code to the consumer of that LibreOffice functionality.

I have already added several patches that limit the maximum array size (Calc says there can be 1+ million or more accessible elements and I limit it to only 1000). Your solution might reduce that a little more if macOS ignores some of the returned elements for something I have already solved in the following patch:

If someone else wants to implement your idea. Cool. It should complement the performance fixes I've submitted to LibreOffice. But I will focus my limited volunteer hours on working through the many other unfixed hanging, crashing, and memory leaking macOS accessibility bugs.
Comment 13 Noel Grandin 2023-06-11 12:34:15 UTC
(In reply to Patrick Luby from comment #12)
> 
> Still don't like my patch to SfxBroadcaster and ConfigurationBroadcaster? :D
> 

I'm nervous, since that piece of code is central to all sorts of stuff and
might make performance worse for other use-cases.

But for now, I suggest we go with your patch and see what happens.

> Seriously though, your proposed solution tries to shift the responsibility

I don't think accessibility trying to instantiate a bunch of objects for every single cell in a large spreadsheet (where 99.99% of them are empty) is a good design, so no, I'm trying to keep it where it belongs.
Certainly accessibility on other platforms does not do this.
Comment 14 Patrick Luby (volunteer) 2023-06-11 12:59:42 UTC
(In reply to Noel Grandin from comment #13)
> 
> I'm nervous, since that piece of code is central to all sorts of stuff and
> might make performance worse for other use-cases.

OK. I understand. Maybe this is a change that needs a wider review? I can ask to put it on the ESC agenda. I have more things to implement down in the vcl/osx/a11y* source files so this particular patch isn't stopping my other work.

> I don't think accessibility trying to instantiate a bunch of objects for
> every single cell in a large spreadsheet (where 99.99% of them are empty) is
> a good design, so no, I'm trying to keep it where it belongs.
> Certainly accessibility on other platforms does not do this.

So I wonder what is so different about macOS? In particular, when VoiceOver is enabled on macOS, clicking on a column sends a native "selection changed" notification to macOS and macOS then sends a request for the selected elements. LibreOffice's macOS accessibility code then calls:

sal_Int64 n = xAccessibleSelection -> getSelectedAccessibleChildCount();

n here will be 1 million+ (i.e. the maximum number of Calc rows). Do other platforms determine the selected child count differently?

AFAICT, the macOS native accessibility code has hardly been touched in 15+ years so maybe the Windows and Linux native accessibility has been refactored over that time?
Comment 15 Noel Grandin 2023-06-11 13:17:38 UTC
(In reply to Patrick Luby from comment #14)
> elements. LibreOffice's macOS accessibility code then calls:
> 
> sal_Int64 n = xAccessibleSelection -> getSelectedAccessibleChildCount();
> 
> n here will be 1 million+ (i.e. the maximum number of Calc rows). Do other
> platforms determine the selected child count differently?
> 

Determining the count is fine - that should be fairly cheap, there is an UNO call to retrieve that, that does not require instantiating all of the child objects.

But actually instantiating all of the accessibility objects for the individual child cells - that is where I think the macOS accessibility layer could maybe do with some improvement.

> AFAICT, the macOS native accessibility code has hardly been touched in 15+
> years so maybe the Windows and Linux native accessibility has been
> refactored over that time?

I don't know about Windows, but certainly myself and Caolan have, over time, whittled away at the worst of the GTK a11y perf issues.

It is worth noting that a lot of these issues were simply invisible 10 years ago, when people in general did not deal with very large documents because it was simply not feasible on the common hardware available back then.

A lot of O(n^2) behaviour stays hidden until the document hits a certain size :-)
Comment 16 Patrick Luby (volunteer) 2023-06-11 23:59:01 UTC
(In reply to Noel Grandin from comment #15)
> 
> But actually instantiating all of the accessibility objects for the
> individual child cells - that is where I think the macOS accessibility layer
> could maybe do with some improvement.
> 

I will take a look at it. I think it may be very useful for reducing cumulative cell objects created (or least reduce the rate of increase).

> It is worth noting that a lot of these issues were simply invisible 10 years
> ago, when people in general did not deal with very large documents because
> it was simply not feasible on the common hardware available back then.
> 
> A lot of O(n^2) behaviour stays hidden until the document hits a certain
> size :-)

Good point. I think that on macOS we are hitting the same limits in other different areas as well. Your lazy instantiation might be a nice improvement on the native end.

The only code I've got in the queue is adding the "max children limit" to the "selected children". Other than that, I'm not seeing any big hangs other than the "close document" performance bottlenecks.

Are there any others that I am missing? Should I create a separate bug for the "close document" performance bottlenecks since it really is a different bug?
Comment 17 Commit Notification 2023-06-12 20:10:23 UTC
Patrick Luby committed a patch related to this issue.
It has been pushed to "libreoffice-7-6":

https://git.libreoffice.org/core/commit/e103f775501dc0a00fca8724b7e4d2117c80a87c

Partial fix tdf#155376 use NSAccessibilityElement instead of NSView

It will be available in 7.6.0.0.beta2.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 18 Commit Notification 2023-06-13 06:13:16 UTC
Noel Grandin committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/f22cb3dfab413a2917cd810b8e1b8f644a016327

tdf#155376 weakly cache ScAccessibleCell

It will be available in 24.2.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 19 Patrick Luby (volunteer) 2023-06-14 12:46:02 UTC
@Noel Grandin implemented a much better fix than my fix for the hang when closing a Calc document when accessibility is enabled:

https://gerrit.libreoffice.org/c/core/+/152932

His fix is now in the nightly build:

https://dev-builds.libreoffice.org/daily/master/current.html

I tested the 2023-06-14 Silicon Mac build this morning (the first nightly build with his fix) and I don't see any hangs or lags in Calc when using the Keyboard Viewer or VoiceOver.

Marking this bug as Resolved/Fixed. Can anyone verify that the bug is fixed for you or if there are any hangs and/or crashes that we still have not fixed?
Comment 20 Commit Notification 2023-06-14 14:05:04 UTC
Noel Grandin committed a patch related to this issue.
It has been pushed to "libreoffice-7-6":

https://git.libreoffice.org/core/commit/c8a8b8e3a03a02c7ee0b616eb728103fb7cefad0

tdf#155376 weakly cache ScAccessibleCell

It will be available in 7.6.0.0.beta2.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 21 Patrick Luby (volunteer) 2023-06-20 17:26:11 UTC
Nightly builds for Mac Intel that includes the fix are now available:

https://dev-builds.libreoffice.org/daily/master/current.html