Use already cached parents when fetching ACL

[PVince81]

When fetching ACL rules, don't refetch the rules from already cached
parents. This fixes performance issues with large folders.

Fixes #1693

Improves performance with 1100 entries from 20 seconds to 4 seconds.

• BUG: because the cache is capped, the logic breaks after cached entries are getting removed by the cap (when testing I increased the capped cache's max)

[PVince81]

Alright, I understand why the cap is messing up: we're putting new rules in a loop and in that loop some earlier entries get discarded. However, in the context of the function, we already assumed that those removed entries were cached.

Would be nice to be able to disable the cap while doing a "transaction" there.

[PVince81]

I managed to fix the problem by reading the already cached rules from the cache, then filling in the missing ones, and then returning. This makes the code independent from the cap logic while the function is running.

[PVince81]

@icewind1991 mind double checking the logic ? I'm not sure if there was a reason why we always wanted to re-query the parents for each request.

[PVince81]

@come-nc I've modified the code according to your review and retested locally.
I'm using $rules now for the cache check because it won't be influenced by the cap while modifying it.

Please have a second look.

[CarlSchwan]

Unit tests are not happy and could you rebase?

[PVince81]

back to draft mode

thanks to @come-nc for pointing out my thinking mistake

I had disabled siblings pre-fetching which is likely why everything got faster.
I've reenabled siblings pre-fetching and now it's up to 12 seconds again.

12 seconds is still better than 20s, so it means my original optimization did help, but it's still too much.

I'll try and understand siblings prefetching better and see if it fits this use case.
If it does fetch all the 1200 entries in one go, it would be nice because then everything gets cached in one go.
But it doesn't seem so, otherwise performance would be even better.

Needs further research...

[PVince81]

in my test case, the 1200 entries are in "sub":

 ▾ $paths = (array [5])
  \
   ⬦ $paths[0] = (string [31]) `__groupfolders/1/sub/folder-100`
   |
   ⬦ $paths[1] = (string [20]) `__groupfolders/1/sub`
   |
   ⬦ $paths[2] = (string [16]) `__groupfolders/1`
   |
   ⬦ $paths[3] = (string [14]) `__groupfolders`
   |
   ⬦ $paths[4] = (string [0]) ``
  /

so the direct parent is always "__groupfolders/1/sub" and is called repeatedly, like 1200 times.

it seems we'd need another kind of cache that tells that we already fetched all rules for the children of this entry

unless we can assume that if the key for "__groupfolders/1/sub" then the keys for the children are already there, which would bring the code back to what it was before the last commit. Not sure if we can assume that though.

[PVince81]

So it seems that in my first attempt, I mistakenly disabled the sibling fetching and performance was already much better, so I wonder if we really need that. Or if there are scenarios (not PROPFIND) where we'd need to fetch the siblings ?

@icewind1991 do you remember the scenarios ?

[icewind1991]

I think it was propfind related with how the properties are/were fetched file by file.

I'm not sure if that still applies though, so it things seem better without it than thats fine by me

[PVince81]

I had a look at getRulesForFilesByParent, hoping that maybe it could fetch all rules in one go and cache them, but the current implementation doesn't do that: https://github.com/nextcloud/groupfolders/blob/master/lib/ACL/RuleManager.php#L159

It will only return rules if they exist instead of returning all the paths, so we can't cache the information "no rules for path X" as a result. This seems to tell me that the purpose of that approach was a different one.

For now in my local env I've added a parentCache in the ACLManager to track whether we already fetched for a given parent.
This reduces the time to 4s, but am still annoyed that getRulesForFilesByPath will still be called for every entry because of the problem described above with getRulesForFilesByParent.

Now, another possible concern: if we do change getRulesForFilesByParent to return all files with empty rules, we'll hit the cache cap within the loop that copies over the rule. This means that getRulesForFilesByPath will still need to be called repeatedly for the missing entries. So the optimization of using getRulesForFilesByPath only makes sense for less than 1000 entries. 🤔

If I don't find a better solution I'll leave it as is, plus the parentCache as it's already quite a step forward in terms of performance.

[PVince81]

After more testing I've observed that getRules() is already called with all the parents, because those are being fetched earlier on, so calling the DB for siblings doesn't make sense any more.

It might be possible to optimize this further by reworking the overall logic, but for the sake of fixing the performance issue (and considering the time already spent on this issue), I'd like to keep this fix minimal.

Therefore, I have now removed the sibling fetching.

[PVince81]

fixed psalm issues, squashed, rebased

[PVince81]

/backport to stable21

[PVince81]

/backport to stable20

[PVince81]

note to self: make sure to properly test in case the conditions for the "sibling fetching" were different there

[PVince81]

/backport to stable22