polls icon indicating copy to clipboard operation
polls copied to clipboard

Optimize the query in the "My polls" section to query only for the polls of the user

Open sorbaugh opened this issue 10 months ago β€’ 31 comments

⚠️ This issue respects the following points: ⚠️

Describe the goal you'd like to achieve

Currently, there seems to be performance issues when opening the "Relevant"/default view if there are lots and lots of polls in the database. It takes a long time to display information in this view, even if the user only has 2 polls. It seems that at the moment everything is loaded in the background despite it displaying only a few).

Describe possible solutions

  • The "My Polls" view should be made the initially displayed view
  • The "My Polls" view should only load those, and not filter for them afterwards. (dedicated query used, so the data can be loaded in an optimized, quicker way)

sorbaugh avatar Apr 22 '24 09:04 sorbaugh

What do you call a lot?

dartcafe avatar Apr 22 '24 16:04 dartcafe

Answer to https://github.com/nextcloud/polls/pull/3444#issuecomment-2076699146

We are facing pressure and urgency due to a support case with a customer that heavily uses Polls and is encountering some performance issues.

My statements to the changes:

  • Make the My Polls app the default view

This affects all users because of the need of a single request. This is not how public software should be developed and we cannot act like this. This could raise possibly a lot of discussion and changes, reverts, changes and so on. This is why I rejected the change. But of course we can can discuss it in a specific, separate issue, what goal is to achieve.

There is a reason, why the template for change requests ask for a user story.

  • Add a spinner as feedback to the user for the loading time

This seems to me like a bug, because the spinner was missing but should be there. So needs a fix and is the reason, why I accepted this PR. Although I already fixed it in a local branch, but to be fair, nobody could know that.

This issue is under maintenance of the nextcloud team and only for the stable-6 branch. The master branch (v7.x) followed another approach to reduce loading times.

If you have input on what could be done to bring the time down to, say, 10 seconds, your input would be invaluable :)

To be honest, 10 seconds for loading a poll list is far fom acceptable. But to understand, where the delay happens I need informations (i.e. this) and the possibility to reproduce, analyse and decide a strategy to solve that problem.

And performace issues do not appear over night, so I don't see a reason for unnecessary acting head over heels. On the other hand I see the need to solve that problem.

dartcafe avatar Apr 25 '24 15:04 dartcafe

PS: If there is need for quick fixes, starting a temporary fork is maybe a better solution, while the issue gets resolved with quailty and time.

dartcafe avatar Apr 25 '24 15:04 dartcafe

I just tried to reproduce this issue. Setup is made by

  • > 1000 polls,
  • no votes or other db entris in the child tables with
  • MySQL 10.5.23,
  • PHP 8.2.18
  • a share hoster,
  • NC29 beta and
  • polls 7.x (current master).

The following measurements are made by

  • a simple reload of the particular page and
  • are hand stopped from reload until full display of all visual items.
  • Browser Firefox dev and Chrome, with deactivated add-ons in a private window

The goal is not to get exact timings, but to get an impression of the affect of different aspects to be able to evolve a useful strategy.

Prenotice: The request for all polls lasted 1,5 sec (Info from the browsers network tab). Acceptable and not the bottle neck, but slow anyway. So the focus lies on the frontend rendering.

Current observations for the records:

Starting situation

  • Load poll list with displaying all polls (Endpoint /all)
    • Firefox 10 sec
    • Chrome 9 sec Load poll list with displaying one polls (Endpoint /open)
    • Firefox 7 sec
    • Chrome 6 sec Load one poll (endpoint /vote/1)
    • Firefox 5 sec
    • Chrome 5 sec

Skip loading of poll items inside the navigation

  • Load poll list with displaying all polls (Endpoint /all)
    • Firefox 5 sec
    • Chrome 5 sec Load poll list with displaying one polls (Endpoint /open)
    • Firefox 2 sec
    • Chrome 2 sec Load one poll (Endpoint /vote/1)
    • Firefox <2 sec
    • Chrome <2 sec

Additionally remove poll items and just display poll title in div

  • Load poll list with displaying all polls (Endpoint /all)
    • Firefox 2 sec
    • Chrome 2 sec Load poll list with displaying one polls (Endpoint /open)
    • Firefox 2 sec
    • Chrome 2 sec Load one poll (Endpoint /vote/1)
    • Firefox 1 sec
    • Chrome 1 sec

Just display poll title in div (Navigation on)

  • Load poll list with displaying all polls (Endpoint /all)
    • Firefox 7 sec
    • Chrome 6 sec Load poll list with displaying one polls (Endpoint /open)
    • Firefox 6 sec
    • Chrome 5 sec Load one poll (Endpoint /vote/1)
    • Firefox 5 sec
    • Chrome 4 sec
/ Poll list, view all polls (Endpoint /all) Poll list, view one poll (Endpoint /open) Poll in vote view (endpoint /vote/1)
unchanged app 10/9 sec 7/6 sec 5 sec
without navigation 5 sec 2 sec <2 sec
without navigation and simple poll list 2 sec 2 sec <2 sec
Just simple poll list 7/6 sec 6/5 sec 5/4 sec

Additional note:

  • Different add-ons could raise the full loading up to 27 seconds

Intermediate conclusion

  • The backend is not the bottle neck, although it is believed, that optimations a re possible
  • Rendering a big amount of poll items in the poll list seems to slowed down the rendering of the complex poll items (2 to 5 seconds for all polls, vs. (2 to 2 seconds with only one poll item in the list)
  • Rendering the navigation with the full store seems to double loading times
  • The navigation also affects the loading time of a single poll in vote view (full app loading), but here the poll is loaded asynchronously. Visually there is no delay in loding the single poll, but it is not usable, while the navigation still loads.
  • Regarding the possible solutions from the OP
    • Changing the default view just has a minimal affect
    • Loading the particular filtered polls instead of filtering in the store may help with some views, but does not solve the underlying performance issue and not for all views. Additionally the performance problem of the navigation is not solvable this way. Both views (poll list and navigation) use the same store.

Possible strategies

tbd

dartcafe avatar Apr 27 '24 11:04 dartcafe

First action taken is to add an option to remove the polls list from the navigation. See #3467 (and #3461 for v7.x)

- Setting Navigation
Loading on grafik grafik
Loading off grafik grafik

dartcafe avatar Apr 30 '24 07:04 dartcafe

For transparency to add some details to the triggering instance for working on performance:

  • > 3000 polls,
  • unknown n.o. votes
  • MySQL 10.5.23,
  • PHP 8.0
  • a dedicated server-setup
  • NC 27.1.9.2
  • polls 6.3.0 beta

AndyScherzinger avatar May 02 '24 15:05 AndyScherzinger

@AndyScherzinger Thanks for the info Could you recognize any performance improvements with the current beta?

dartcafe avatar May 03 '24 20:05 dartcafe

@dartcafe I don't have exact numbers, but yes. I can't say specifically about 6.3.0 but a test build we provided - let's call it 67.2.0 by @come-nc - seemed to ease the situation (while not enough to resolved the matter completely).

So releasing 6.3.0 would be great to update instances to a non-beta version and work further from there, if needed.

AndyScherzinger avatar May 04 '24 07:05 AndyScherzinger

I don't have exact numbers, but yes.

Great. I don't need exact mesaurements, it is just because getting some feedback. I guess @come-nc build it from the stable-6 branch.

seemed to ease the situation (while not enough to resolved the matter completely)

Yeah, sure. There were just a few quick actions taken. Another is in the way #3477. And I bet, there is more room for improvements.

I would release another beta before the final release, just to make sure, everything works and maybe some more progress can be felt.

dartcafe avatar May 04 '24 07:05 dartcafe

I would release another beta before the final release, just to make sure, everything works and maybe some more progress can be felt.

Sounds like a good plan, yeah πŸ‘ I'll aks for getting it tested out but can't promise anything yet. However @come-nc or @ArtificialOwl might be able to test the then.

AndyScherzinger avatar May 04 '24 08:05 AndyScherzinger

I would release another beta before the final release, just to make sure, everything works and maybe some more progress can be felt.

@dartcafe any particular date you might have in mind? Any day before the end of this week for the final release would be extremely helpful.

AndyScherzinger avatar May 06 '24 06:05 AndyScherzinger

BTW: Are you able to calculate the backend performance. @ChristophWurst had some telematic data in #3362.

I want to understand, if it is valuable to look for further optimizations at the backend side. Another approach is to add a lazyloding to the frontend, so that the list is not rendered completely, but just for the visible part.

I think the responsability takes its prize. Maybe I did something wrong but the differences between a simple list and a list of components is recognizable.

dartcafe avatar May 06 '24 07:05 dartcafe

@dartcafe any particular date you might have in mind? Any day before the end of this week for the final release would be extremely helpful.

I would like to add another PR, which effectly targets the redering performance inside the navigation and the polls list. Just testing for the master branch, before I add the PR to the repo.

Anyway #3477 and the comming PR ahev to be backported to the stable-6 branch.

dartcafe avatar May 06 '24 07:05 dartcafe

See the frontend changes here #3479

dartcafe avatar May 06 '24 07:05 dartcafe

BTW: Are you able to calculate the backend performance. @ChristophWurst had some telematic data in https://github.com/nextcloud/polls/issues/3362.

can't say for sure but afaik @ArtificialOwl did setup 3K polls locally for testing, so maybe he know a bit about that.

For not adding too much more in terms of change-set, I would say best to release 6.3.0 "as-is" as long as it contains the optimizations already done as well as the changes for the web interface in terms of "showing spinners while loading" and "making my polls a default view".

If that is the case anything new could be backported of course but than shipped as 6.4.0?

What do you think @dartcafe ?

Also @come-nc - do you know if the above mentioned changed are present on stable-6?

AndyScherzinger avatar May 06 '24 10:05 AndyScherzinger

Times after applying #3477 and #3479 all are similair between 2 and 3 seconds for loading the whole app.

dartcafe avatar May 06 '24 10:05 dartcafe

Also @come-nc - do you know if the above mentioned changed are present on stable-6?

"making my polls a default view" was reverted as it was not helping performance and was refused design-wise by @dartcafe .

"showing spinners while loading" and other perf improvements are present in 6.3.0 current state.

come-nc avatar May 06 '24 10:05 come-nc

@AndyScherzinger If the current situation suits your needs, I am fine with releasing. I would prefer to concentrate on the master branch.

dartcafe avatar May 06 '24 11:05 dartcafe

@ArtificialOwl Could you "feel" some pleasing change with the current status (6.3.0-beta1)?

dartcafe avatar May 06 '24 11:05 dartcafe

But slowly I think backporting the master to the stable-6 would have been a more straightward job. πŸ˜‰ @come-nc removed the main incompatibilities with his initial commits.

dartcafe avatar May 06 '24 11:05 dartcafe

But slowly I think backporting the master to the stable-6 would have been a more straightward job. πŸ˜‰

Yes, for sure. So I guess we'll have to see and might face further backports in the near near future, but can handle that - I am sure πŸ‘

AndyScherzinger avatar May 06 '24 11:05 AndyScherzinger

Also dropped you a message on Talk, can also switch to mail or else if that is more suitable for you @dartcafe - and works in German πŸ˜†

AndyScherzinger avatar May 06 '24 11:05 AndyScherzinger

@AndyScherzinger I would try to get the next beta (better RC) tomorrow with the chunked loading, since it has a big impact on the rendering time. But I need @come-nc for that, so he is aware of what happens there.

The release will be possible shortly after it (I would target Wednesday, because of the holidy here on Thursday). But @v1r0x has to be ready for the appstore release, since I do not have access to it.

dartcafe avatar May 07 '24 19:05 dartcafe

@come-nc is off tomorrow, public holiday but back on Thursday. My customer deploys next Tuesday so a final on Monday is needed on my end for 6.x. Would that be possible? Any fΓΌr there's changes could also go into a 6.4 I believe. I know that also @artonge wanted to work on.

Also not sure if you'd meant this wwwk Thursday for the final or next week Thursday. Hence my point with the final on Monday latest.

AndyScherzinger avatar May 07 '24 19:05 AndyScherzinger

@AndyScherzinger I added the last improvement in #3492 This should speed up the redering a lot. Additionally I adopted the dom optimisations which found their way to the master before this action. I have the impression it helps a little bit too.

I will build the beta from https://github.com/nextcloud/polls/tree/backport/3479/stable-6

As soon I get an OK from you I will build the release from this branch, too.

@artonge I'll wait with the merge until you had the chance to check the compatibility to #3490

dartcafe avatar May 08 '24 05:05 dartcafe

@dartcafe I pinged some frontend people since @artonge is off for the rest of the week. So I think #3490 might just need to come later or be adapted to your PR.

AndyScherzinger avatar May 08 '24 07:05 AndyScherzinger

#3490 is not planned for 6.3 since I want to know, what side effect may occur.

dartcafe avatar May 08 '24 09:05 dartcafe

Ah, okay. I misunderstood it then πŸ‘

AndyScherzinger avatar May 08 '24 09:05 AndyScherzinger

All looking good from my end @dartcafe πŸ‘

AndyScherzinger avatar May 08 '24 13:05 AndyScherzinger

OK. Then I will fire the release now.

dartcafe avatar May 08 '24 15:05 dartcafe