CollaboraOnline / online

Collabora Online is a collaborative online office suite based on LibreOffice technology. This is also the source for the Collabora Office apps for iOS and Android.
https://collaboraonline.com
Other
1.8k stars 690 forks source link

calc: performance with lots of comments #6911

Closed mmeeks closed 1 year ago

mmeeks commented 1 year ago

We have fairly horrible performance scrolling around documents with large numbers of comments:

perf-23 05-calc-comments

There are several sections to this:

load time

we do way too much work sizing and re-sizing drawing layer objects - which ultimately turn into a red rectangle.

render time

50% of tile render time is doing 'SdrPageView::DrawLayer' - the effect of this (I guess) is to render the small red rectangles denoting that "cell has comment". We should I guess avoid this and track shown/hidden comments elsewhere (they are almost always not shown) and ideally since they are rendered in the HTML in a popup anyway not do any of this rendering slowness.

page-down time

as we change the viewport we emit: OUTGOING: commandvalues command=.uno:ViewAnnotationsPosition which results in - a big reply for the viewport:

commandvalues: { "commentsPos": [ { "id": 118, "tab": 0, "cellPos": "1296, 60624, 5831, 431"}, { "id": 1, "tab": 0, "cellPos": "8292, 1584, 1883, 443"}, { "id": 2, "tab": 0, "cellPos": "8292, 2028, 1883, 431"}, { "id": 3, "tab": 0, "cellPos": "8292, 2892, 1883, 431"}, { "id": 4, "tab": 0, "cellPos": "8292, 3324, 1883, 431"}, { "id": 6, "tab": 0, "cellPos": "8292, 3756, 1883, 431"}, { "id": 7, "tab": 0, "cellPos": "8292, 4188, 1883, 431"}, { "id": 8, "tab": 0, "cellPos": "8292, 4620, 1883, 431"}, { "id": 9, "tab": 0, "cellPos": "8292, 5052, 1883, 431"}, { "id": 10, "tab": 0, "cellPos": "8292, 5484, 1883, 431"}, { "id": 11, "tab": 0, "cellPos": "8292, 6348, 1883, 431"}, { "id": 12, "tab": 0, "cellPos": "8292, 7212, 1883, 431"}, { "id": 13, "tab": 0, "cellPos": "8292, 7644, 1883, 431"}, { "id": 14, "tab": 0, "cellPos": "8292, 8076, 1883, 431"}, { "id": 15, "tab": 0, "cellPos": "8292, 8508, 1883, 431"}, { "id": 16, "tab": 0, "cellPos": "8292, 8940, 1883, 431"}, { "id": 17, "tab": 0, "cellPos": "8292, 9372, 1883, 431"}, { "id": 18, "tab": 0, "cellPos": "8292, 10236, 1883, 431"}, { "id": 19, "tab": 0, "cellPos": "8292, 10668, 1883, 431"}, { "id": 20, "tab": 0, "cellPos": "8292, 11964, 1883, 431"}, { "id": 21, "tab": 0, "cellPos": "8292, 12396, 1883, 431"}, { "id": 22, "tab": 0, "cellPos": "8292, 13260, 1883, 431"}, { "id": 23, "tab": 0, "cellPos": "8292, 13692, 1883, 431"}, { "id": 24, "tab": 0, "cellPos": "8292, 14124, 1883, 431"}, { "id": 25, "tab": 0, "cellPos": "8292, 14556, 1883, 431"}, { "id": 26, "tab": 0, "cellPos": "8292, 14988, 1883, 431"}, { "id": 27, "tab": 0, "cellPos": "8292, 15420, 1883, 431"}, { "id": 28, "tab": 0, "cellPos": "8292, 15852, 1883, 431"}, { "id": 29, "tab": 0, ....

creating this (cf. the profile) is extremely expensive - cf. 'getCommandValues' - which seems to spend most of its time correcting for the pixel <-> TWIPS nonsense. We should instead return a set of cell co-ordinates, and do that conversion in the client side (where we're quite good at it ;-). Quite probably we should also render the 'has a note' overlay in the client too - so we can make that easier to interact with and more accessible. Then disable the comment rendering on the server side for the LOK mode.

Thanks ! =)

caolanm commented 1 year ago

mildly amusingly, the first place I look at that uses a comment cellPos to check for an existing comment on inserting a new one seems to be broken. https://github.com/CollaboraOnline/online/pull/6980 to fix that

caolanm commented 1 year ago

[comments.ods](https://github.com/CollaboraOnline/online/files/12239197/comments.ods)

test file with various cases like merged cells, frozen cols/rows, hidden cols/rows and RTL sheets

caolanm commented 1 year ago

step 1: https://gerrit.libreoffice.org/c/core/+/155044 and https://github.com/CollaboraOnline/online/pull/6988 to move to use a set of cell co-ordinates for these notes/comments/postits.

caolanm commented 1 year ago

core flamegraph of 100 pagedowns and 100 pageups on calc document with plenty of comments, before and after before after

caolanm commented 1 year ago

step 2: draw the note indicators browser side instead of server side: https://gerrit.libreoffice.org/c/core/+/155223 and https://github.com/CollaboraOnline/online/pull/7001

caolanm commented 1 year ago

step 3: I still see lots of DrawSelectiveObjects still-lots-of-DrawSelectiveObjects

but with https://gerrit.libreoffice.org/c/core/+/155268 and https://gerrit.libreoffice.org/c/core/+/155269 in place I get

opt-DrawSelectiveObjects

caolanm commented 1 year ago

The specific ideas here are done now