GridView Performance Degradation with Multiple Roles and Workspaces

[saschabaecher-twocream]

Steps to reproduce

1. Create a user
2. Create at least 5 roles and assign multiple workspaces to each role
3. Build a folder structure with several subfolders
4. Add a large number of objects to the folder structure
5. Open the GridView
6. A significant slowdown should be noticeable

Blackfire Results

Roles: 8
Workspaces: 50 (partially duplicated)

As an admin, the process is much faster since workspace queries are not included:

Actual Behavior

The loading times are extremely long and can reach up to 20 seconds or more, depending on the configuration, often leading to timeouts that make the GridView unusable.

Expected Behavior

Faster SQL queries to significantly reduce loading times, avoid timeouts and ensure that the GridView remains usable without long waiting times.

[fashxp]

related to pimcore/pimcore#12776

[mattamon]

@saschabaecher-twocream I tried a first improvement here #748 in combination with pimcore/pimcore#17834 it improved my performance already a lot even though I only have a small set of data.
It would be awesome if you could try it for yourself if it is also an improvement for you.

[saschabaecher-twocream]

Hi @mattamon - I worked on it on Monday and had the same solution as you! It reduces the times extremely, a very powerful optimisation.

I was going to post a PR about it later this week.

Thanks for checking it out too! Sometimes there are just coincidences ;)

[mattamon]

Coincidence or using the same tools :D

And no problem I will reopen your Issue still, cause I want to close it once the PR is merged and fully tested :)

[mattamon]

@saschabaecher-twocream as @fashxp pointed out the query does not work since it results in false positives.

I changed it now with only a micro-optimization. Maybe you could test it in combination with the index if the performance for you is better.

[kingjia90]

This topic is quite prone to false positive and negatives, telling by first-hand experience 🤕

I'd like suggest that we take a TTD approach and have the tests set in place first to ensure that behavior is always consistent and as expected/intended, then we can optimize,do benchmarks and experiment, with some more ease of mind.

[saschabaecher-twocream]

Hi @mattamon - Thanks for taking a closer look at this issue! I've integrated and tested the version from #748. The results look good so far, and performance has improved enough with the indexes that it's workable for now.

[kingjia90]

I'd like to propose these changes, #767
I am confident that it would be providing right results, be consistent (as it is the same as something tested that we use elsewhere already) and the performance should be improved. Could you please have a look and test it?

[kingjia90]

Closing as resolved by #767

[jdreesen]

Without tests? 😢

[kingjia90]

@jdreesen yup, unfortunately not for the moment due to other priorities/deadlines (you may know, before the winter holidays 😄 ).

I am confident that it would work perfectly fine, because it's the same condition taken from something already well covered and tested.
Let's see it as a sort of compromise, prioritizing in having it returning right and consistent results with an assuredly performance increase in a bugfix, despite being aware there are plenty room for improvement.

Opening a separate followup issue #768, if someone can join and take over for tests, please tag me in, i would be glad to help reviewing

[BlackbitDevs]

@kingjia90 I see you simply copied my changes concerning grid performance from pimcore/pimcore#12776 to #700 - but what is with the other places:

• child loading in tree
• asset list loading

And btw. it is a bit frustrating that you copy my changes to a new PR. We have put a lot of unpaid work into this to make Pimcore better.
But by copying the changes to another PR, the runtime comparison from pimcore/pimcore#12776 is gone, our reputation etc. Actually for this reason I always enable the checkbox that core maintainers are allowed to modify my PRs.

[kingjia90]

@BlackbitDevs There must be an unfortunate misunderstanding, i didn't intentionally nor specifically copy the changes in your former PR, i actually just copied something that i have introduced in the past (thus i was very familiar with) and tweaked a little bit, and casually happened to match in parts of what you have suggested over there in the past PR.

I have just focused on the grid, because there not only it's slow, but that "2 subqueries OR condition" is also known to returning wrong results, making it possible to merge in a bugfix release, rather than in the next minor.

The child loading in tree is just fine as is in the current state, they have an extra "rule/logic" under the hood, due to the list permission to allow navigation via tree, so it wasn't fully applicable at that state in the other PR

From a technical pov, I had to open a new branch/PR here anyway because there were some decoupling in different repositories (from core to admin ui) and the branch you worked on was too outdated by now and resolving conflicts,rebasing would have been unnecessary tricky and time consuming.

I didn't mean to frustrate anyone by "stealing credit" nor overshadow anyone's valuable work and contribution, or anything like that.
I personally highly appreciate all the help, inputs and interactions and it's definitely the favorite part of the job.

Hope you would accept my sincere apologies for causing any distress and believe me that i am in good faith and it is just an unfortunate coincidence and i didn't realize on time that it could led to this.

[BlackbitDevs]

@kingjia90 All good. The most important point is that Pimcore got better! Thanks for your explanation.

[cancan101]

@kingjia90 do you mind adding a note pimcore/pimcore#12776 (comment) as to the limitations of this solution vs what was proposed initially?

You had written:

Closing here for the moment in favor of a quicker fix of

which makes me think the fix that went in was not as broad as what was initially proposed.

[kingjia90]

@cancan101 i would be very glad to add some more notes

The sql condition snippet was applied in every place where permission is checked, which turned out it was not completely right (making tests fail) as there are places like the element trees where it needs a different approach due to their nature.
AND
Overall the repeated code (not only the mentioned PR but in the current code base as well) makes it hard to maintain/customize/extend, taking also into consideration that the same snippet is also copied in other places like bundles/repos too and share the same base but may have slight differences, which would at some point create unwanted inconsistencies.

The ideal PR for a future minor release would be centralizing and unifying these approaches into a service or helper in the Core, bump to require this core version from any external bundle using this permission check, and of course, even have the tests for the grid, all this while avoiding bc-breaks nor behavior changes and experiment new ways to increase performances even more (opensearch? caching? etc..). That can really get broad 😄 The word "quicker" was to refer that by doing it as a bugfix without refactoring would solve the issue in the immediate, as i mention above, it was not just slow, but it was also known to be buggy under certain circumstances.

[fashxp]

also keep in mind, that with Pimcore studio we will follow a complete different approach based on the generic data index (opensearch, elasticsearch) with much better performance.