calc perf: Note listing

[mmeeks]

From this week's calc flamegraph:

https://user-images.githubusercontent.com/833656/271274520-6345ee72-2a06-402e-a12c-72bd8d49df8d.svg

Shows a chunk of time when users join the sheet is interrogating the whole document as to whether notes are present:

6% of the time - is searching in empty columns far outside the range of the sheet that contains data I expect - in order to see if those columns have notes - inevitably they do not. 16000 is a lot of columns to check to see if there is something there - when there really isn't almost all the time.

I wonder if we should tweak our implementation to shrink the real sheet column and row count to something more reasonable - and then expand that on user interaction: adding row/columns with some '+' button, or pasting into them in some way (?) though that's a bigger pain.

Failing that, having and maintaining some 'last note column' or other better data-structure for notes than columnar storage might be helpful.

[mmeeks]

There is a ridiculous amount of this search going on; I assume that the result is:

1697730362525 INCOMING: commandvalues: { "commentsPos": [ { "id": 1, "tab": 0, "cellRange": "0 0 0 0"}]}

I seem to get this when switching tab, but worse - when I move between cells with the arrow keys (perhaps just when scrolling the window) - that's a pretty heinous cost to pay for this small operation - if that is indeed what's going on :-) @caolanm I would assume there is some legacy fetching or pushing going on there from when notes were pushed not in their entirity but cropped to the viewpane or (?).

[mmeeks]

Seems like we do far too much work as we extend the document - by moving into 'blank space' - that's when we emit (from the client) this OUTGOING: commandvalues command=.uno:ViewAnnotationsPosition which is something of an anomaly: much of the data we need is helpfully pushed:

1697732170923 OUTGOING: key type=up char=0 key=1031
keyboard handler: keydown PageDown 0 undefined KeyboardEvent {isTrusted: true, key: 'PageDown', code: 'PageDown', location: 0, ctrlKey: false, …}
1697732170997 OUTGOING: key type=input char=0 key=1031

... getting page down ...

1697732147269 INCOMING: invalidateheader: all
1697732147269 OUTGOING: commandvalues command=.uno:ViewAnnotationsPosition -> why is this not pushed ? ...
1697732147270 INCOMING: cellselectionarea: EMPTY
1697732147271 INCOMING: referencemarks: { "marks": [ ] }
1697732147271 INCOMING: graphicselection: EMPTY
1697732147272 INCOMING: invalidatetiles: EMPTY, 1, 0 wid=208

[mmeeks]

We probably need to re-visit these:
commit 3af2346
Author: Dennis Francis dennis.francis@collabora.com
Date: Fri Jan 17 16:59:02 2020 +0530

Request comment positions when zooming/view reset

The twips coordinates sent by core in CommentsPos message are already
correct w.r.t zoom level (See ScModelObj::getPostItsPos() in core.git).
In addition these values are view specific (each client user).

Change-Id: I18c2971f34362de0eba5181f9dbe3e662c223575

commit ef53761
Author: Marco Cecchetti marco.cecchetti@collabora.com
Date: Tue May 2 10:51:16 2017 +0200

Loleaflet: Fix for row/col headers and annotation marks on tab switching

Change-Id: Id0f52603af7eaa49979f68d222c9c94e79cf65b1

commit 647556b
Author: Marco Cecchetti marco.cecchetti@collabora.com
Date: Tue Apr 25 18:57:30 2017 +0200

loleaflet: misplaced comment mark on inserting/deleting/resizing row/col

Change-Id: Id0f52603af7eaa49979f68d222c9c94e79cf65b1

in light of:

commit 4d7f7d6
Author: Caolán McNamara caolan.mcnamara@collabora.com
Date: Mon Jul 31 11:31:12 2023 +0100

cool#6911 calc comment positions are now cell addresses

Signed-off-by: Caolán McNamara <caolan.mcnamara@collabora.com>
Change-Id: I0a70f1801f519df066ff5bc9dd57a5d485967b61

I expect we would do well to fetch comment positions either just for the current sheet - which should save a lot of time; but we should prolly check the mobile UI for comments (?) - and we should re-visit when we need to refresh comments. Particularly when scrolling and/or expanding the sheet area - we shouldn't look at what we're fetching.

Finally - we should -probably- push this thing from the online/kit side as we do for these other settings, rather than taking another round-trip to fetch it later :-)

[caolanm]

I'm working on this, with the integration of the "comment positions are now cell addresses" I turned the addresses to positions quite early and kept thing similar to how they were. We should be able to keep them as addresses and work on those and then I think we can drop the round-trip to "update positions".

[mmeeks]

Seems like the micro-optimization we did got this cost down to ~2.5% in the 2nd calc profile here:
https://user-images.githubusercontent.com/833656/278306859-4cf8804b-4f60-4ac5-90e2-28c4e59f2867.svg but I think we can go lower without too much work :-)

If we get some easy wins, we should prolly move on to the other costs in doc_setView - which is 15%+ =)

Thanks ! =)

[caolanm]

right, turns out that removing these calls to make core to send the "full updates" of comments reveals that when we add/remove a comment then the little update that core sends has the comment id quoted while the comment ids in the "full update" are not quoted, so they are then not found. Which is a known problem of boost::property_tree::ptree used for the little update vs the tools::JsonWriter used for the full updates.

So, https://gerrit.libreoffice.org/c/core/+/158560 as the prerequisite for the remainder

[caolanm]

I think we can close this, always more that can be done, but the major issue seems resolved now