search

letsfindaway / OpenBoard

I'm using this fork to contribute features and fixes to the upstream project. In order to create good pull requests, I'm rebasing my feature branches, squashing and reordering commits, etc. If you fork this repository be aware that my development branches may rewrite history without prior notice.
http://openboard.ch/
GNU General Public License v3.0
9 stars 0 forks source link

[Bug] Huge performance issues coming with 1.7.0 and 87df649 (but not only) #164

Closed kaamui closed 3 days ago

kaamui commented 6 months ago

Hi Martin,

a teacher juste shared me a document where he was experiencing some loss of data when exiting Openboard (I'll write an issue about that as well, it's an issue with the PersistenceWorker). During my testing, I was surprised by the loading time of one of the pages of his document. I started some comparisons about it, and found that there is a huge performance issue with large pages like this one. Unless you have an idea of what could explain it, I'll have to revert this commit for 1.7.1, until we find the answer. Note that this commit is not the only one causing such bad performances, but I didn't identify the others yet.

Describe the bug

Huge performance issues are coming with 87df649 fixes.

To Reproduce

  1. Import the attached UBZ

Expected behavior

A clear and concise description of what you expected to happen.

The document loads quickly.

Actual behavior

A clear and concise description of what actually happened

I didn't identify where the other 4s of loading time are coming from at this point.

Screenshots

If applicable, add screenshots to help explain your problem.

Additional resources

PDF, images, sounds... anything useful to reproduce the bug 13.02.24 1346.zip

Context

Additional context

kaamui commented 6 months ago

Here is the original document. 6.02 gravitation.zip

The first page is way heavier (2.2Mo where page 4 is 824Ko) but loads faster. Seems that groups are consuming.

kaamui commented 6 months ago

Also, the vast majority of the time is spent here : https://github.com/letsfindaway/OpenBoard/blob/fff1a15ef00c1428c78db498d9ca63172b5579d9/src/adaptors/UBSvgSubsetAdaptor.cpp#L991-L994

letsfindaway commented 6 months ago

Yes, this is also my experience, and finally in

https://github.com/letsfindaway/OpenBoard/blob/fff1a15ef00c1428c78db498d9ca63172b5579d9/src/domain/UBGraphicsScene.cpp#L305-L313

which is quadratically slow, because it iterates over all existing items. It is called from

https://github.com/letsfindaway/OpenBoard/blob/fff1a15ef00c1428c78db498d9ca63172b5579d9/src/domain/UBGraphicsScene.cpp#L1948-L1955

This means that the statement in the comment is not true. The z Level is not DEFAULT_Z_VALUE.

letsfindaway commented 6 months ago

The commit adds these lines: https://github.com/letsfindaway/OpenBoard/blob/fff1a15ef00c1428c78db498d9ca63172b5579d9/src/adaptors/UBSvgSubsetAdaptor.cpp#L2380-L2383

I thought I would have to assign the z value of a group, if the item is in a group. Should I better leave it at Default, so that the if statement does not call zLevelAvailable?

For me this speeds up loading by about the factor 12 / 5, but I don't know what the correct handling would be.

But perhaps removing these lines again instead of reverting the whole commit would be sufficient.

kaamui commented 6 months ago

I'm not sure either. As we didn't have known issues of z-level for grouped items, I think I'll revert.

I prefer reverting the whole thing and revert it back after we fine-tuned it, because I don't want to let potentiel other issues not detected yet. Do you see potential issues reverting it ?

I also noticed that the number of detected strokes differ, with or without this commit, but I didn't investigate.

letsfindaway commented 6 months ago

I'm not sure either. As we didn't have known issues of z-level for grouped items, I think I'll revert.

I prefer reverting the whole thing and revert it back after we fine-tuned it, because I don't want to let potentiel other issues not detected yet. Do you see potential issues reverting it ?

Yes, the duplication of the lines, e.g. the cross of the compass tool.

I also noticed that the number of detected strokes differ, with or without this commit, but I didn't investigate.

kaamui commented 6 months ago

I'm not sure either. As we didn't have known issues of z-level for grouped items, I think I'll revert. I prefer reverting the whole thing and revert it back after we fine-tuned it, because I don't want to let potentiel other issues not detected yet. Do you see potential issues reverting it ?

Yes, the duplication of the lines, e.g. the cross of the compass tool.

I also noticed that the number of detected strokes differ, with or without this commit, but I didn't investigate.

OK. Let's try to come to correct performances without reverting all the good changes that have been done.

Do you see a reason why it would still take 5s instead of the second took by 1.6.4 ? I thought it could be the "real-time" thumbnails but it doesn't seem to.

You seem to have better tools/skills than me to profile execution times of each function ... what do you use ?

letsfindaway commented 6 months ago

I'm using the profiler built in to QtCreator, which again is using some of the standard Linux profiling tools.

No, is do not have an idea why it takes 5 sec, but still i See that much time is spent in that zLevelAvailable function. I would probably recommend to tune this, e.g. by using a QSet of used z values, where lookup is fast.

Do you have some insight in the zLevel handling?

For now I have to stop investigating as I have to pick up my wife at the station.

kaamui commented 6 months ago

Do you have some insight in the zLevel handling?

Not much, just that now that you say it, the way available zLevel values are searched is just awful ... what do you want to know precisely ?

Thank you for your help

kaamui commented 6 months ago

Here are some documents where Z layers are very important : Elec_Exo10-11.zip 1_Elec_FiAct_SC_20-21.zip

kaamui commented 6 months ago

Something like that should perform better. https://github.com/OpenBoard-org/OpenBoard/pull/892/commits/8870cfaec63d3a0bdfa71370bfea895bddb3f3db

I don't even understand why we would need more than that. The ZLayerController does more, but I don't see why available zLevel values could not be handled like this.

letsfindaway commented 6 months ago

Something like that should perform better. OpenBoard-org@8870cfa

Except that you should return !contains instead of contains.

kaamui commented 6 months ago

Yep, I'll fix it tomorrow, I just wanted to gîve a start to this refactoring and thought that we would both be able to work on the draft PR. Can you or is the original author the only one able to change it ?

letsfindaway commented 6 months ago

Yep, I'll fix it tomorrow, I just wanted to give a start to this refactoring and thought that we would both be able to work on the draft PR.

I tested it and performance-wise is is a huge step and could do the factor of 5.

Can you or is the original author the only one able to change it ?

I think I could push to this branch, but I like to avoid it and only push to my fork. I could then even make PRs for updates of your PR :)

BTW: I'm not available from Thursday (tomorrow) until Monday next week.

letsfindaway commented 3 days ago

See https://github.com/OpenBoard-org/OpenBoard/pull/892