-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize the query in the "My polls" section to query only for the polls of the user #3439
Comments
What do you call a lot? |
Answer to #3444 (comment)
My statements to the changes:
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.
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.
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. |
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. |
I just tried to reproduce this issue. Setup is made by
The following measurements are made by
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
Skip loading of poll items inside the navigation
Additionally remove poll items and just display poll title in div
Just display poll title in div (Navigation on)
Additional note:
Intermediate conclusion
Possible strategiestbd |
For transparency to add some details to the triggering instance for working on performance:
|
@AndyScherzinger Thanks for the info |
@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. |
Great. I don't need exact mesaurements, it is just because getting some feedback.
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. |
Sounds like a good plan, yeah 👍 |
@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. |
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. |
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. |
See the frontend changes here #3479 |
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? |
"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. |
@AndyScherzinger If the current situation suits your needs, I am fine with releasing. I would prefer to concentrate on the master branch. |
@ArtificialOwl Could you "feel" some pleasing change with the current status (6.3.0-beta1)? |
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. |
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 👍 |
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 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. |
@come-nc is off tomorrow, public holiday but back on Thursday. 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 I added the last improvement in #3492 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 |
#3490 is not planned for 6.3 since I want to know, what side effect may occur. |
Ah, okay. I misunderstood it then 👍 |
All looking good from my end @dartcafe 👍 |
OK. Then I will fire the release now. |
See #3497 |
I will close this issue, since optimizations werde done with another approach. |
Sounds good to me. FYI - @ArtificialOwl will keep working on performance improvements which we need to also backport to v6 for Nc27 |
As far as I understood, @ArtificialOwl wnats to try to backport the current master to NC27. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
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 text was updated successfully, but these errors were encountered: