FIX 11577 performance issues with large list of groups by beerbohmdo · Pull Request #11578 · silverstripe/silverstripe-framework

beerbohmdo · 2025-01-27T10:04:01Z

Description

If you have many groups you can no longer edit a member in the admin.

Manual testing steps

As this is a performance issue, I am not sure howto create a unittest for this. But here is a task to create a massive amount of groups:

<?php

use SilverStripe\Dev\BuildTask;
use SilverStripe\ORM\DB;
use SilverStripe\Security\Group;

class GroupCreationTask extends BuildTask
{
    public function run($request): void
    {
        DB::query('TRUNCATE "Group"');
        DB::query('TRUNCATE "Permission"');

        Group::singleton()->requireDefaultRecords();

        $conn = DB::get_conn();
        $conn->transactionStart();

        $start = hrtime(true);

        for ($i = 0; $i < 3000; $i++) {
            if (($i % 100) == 0) {
                $conn->transactionEnd();
                $conn->transactionStart();

                $end = hrtime(true);
                $t = ($end - $start) / 1e9;

                printf("%d - %fs\n", $i, $t);

                $start = hrtime(true);
            }

            $parentGroup = Group::create();
            $parentGroup->Code = 'group_' . $i;
            $parentGroup->Title = 'Group ' . $i;
            $parentGroup->write();

            for ($j = 0; $j < 5; $j++) {
                $group = Group::create();
                $group->Code = 'group_' . $i . '_' . $j ;
                $group->Title = 'Group ' . $i . ' ' . $j;
                $group->ParentID = $parentGroup->ID;
                $group->write();
            }
        }

        $conn->transactionEnd();

        $end = hrtime(true);
        $t = ($end - $start) / 1e9;

        printf("%d - %fs\n", $i, $t);
    }
}

Issues

Group tree generation timeouts when you have a massive amount of groups #11577

Pull request checklist

The target branch is correct
- See picking the right version
All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
- Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
The commit messages follow our commit message guidelines
The PR follows our contribution guidelines
Code changes follow our coding conventions
This change is covered with tests (or tests aren't necessary for this change)
Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
CI is green

GuySartorelli

Thanks for submitting this - I've requested a few changes below.

FYI the beta release for CMS 5.4 (which is also the feature freeze for all of CMS 5) is currently scheduled for Feb 17, so for this change to be included in CMS 5 it will need to be merged before then.
I will do my best to be quick to respond and review this PR as you update it.

GuySartorelli · 2025-02-03T21:24:55Z

src/Security/Group.php

+    private static $searchable_fields = [
+        'Title',
+        'Description',
+    ];


I don't think this change should alter the searchable fields

Without the default search context is empty and the SearchableDropdownField does not filter.

GuySartorelli · 2025-02-03T21:25:07Z

src/Security/Group.php

+        $threshold = DBForeignKey::config()->get('dropdown_field_threshold');
+        $overThreshold = $groups->count() > $threshold;


We shouldn't be using that unrelated configuration here.

This are the default settings for SearchableDropdownField. I can create a extra config for that.

GuySartorelli · 2025-02-03T21:26:06Z

src/Security/Group.php

+    public function getBreadcrumbTitle(): string
+    {
+        return $this->getBreadcrumbs(' > ');
+    }


Why do we need this new method?

This is the Label for the FormField.

GuySartorelli · 2025-02-03T21:26:40Z

src/Security/Group.php

    private function dedupeCode(): void
    {
-        $currentGroups = Group::get()
-            ->exclude('ID', $this->ID)
-            ->map('Code', 'Title')
-            ->toArray();
        $code = $this->Code;
        $count = 2;
-        while (isset($currentGroups[$code])) {
-            $code = $this->Code . '-' . $count;
-            $count++;
+
+        if ($code) {
+            while ($this->checkIfCodeExists($code)) {
+                $code = $this->Code . '-' . $count;
+                $count++;
+            }
+
+            $this->setField('Code', $code);
        }
-        $this->setField('Code', $code);
+    }
+
+    private function checkIfCodeExists(string $code): bool
+    {
+        return Group::get()->filter('Code', $code)->exclude('ID', $this->ID)->exists();
    }


How do changes to the deduping code relate to usage of a different form field?

This is more or less the same issue as with the large list in the FormField. Without you are not able to save a group if you have many of them.

You can check the output of my generater script above with and without the change. Without the script will get slower with each iteration.

GuySartorelli · 2025-02-03T21:27:45Z

src/Security/Member.php

+                $threshold = DBForeignKey::config()->get('dropdown_field_threshold');
+                $overThreshold = $groups->count() > $threshold;


Like with the change in Group, we shouldn't use this unrelated configuration.

beerbohmdo · 2025-02-05T08:10:17Z

I created custom configs, a getter for a custom search context and added some comments.

GuySartorelli · 2025-02-18T20:06:49Z

Hi @beerbohmdo,

Sorry, I haven't had a chance to look at this recently and it's likely I won't get to in time for the 5.4.0 beta, which was originally scheduled to release on Monday but has been delayed slightly.

This will likely need to target 6.0 to be included in the CMS 6.0 major release unfortunately.

lekoala · 2025-03-24T10:31:40Z

hi @beerbohmdo and @GuySartorelli

I'm seeing the PR is a bit related to an issue I have here
#11645

In my opinion, making count queries on large tables to check if over threshold is really counter productive, because making count queries on large tables is slow.

I also see that this PR adds a setting very similar to what is in place in DBForeignKey (and if anything, I don't think having multiple dropdown_field_threshold settings is a good thing)

Instead, I would really suggest adding a "is_large_table" on the DataObject that could be checked by any functionality that needs to determine if it needs to enable lazy loading or not

This could unify fixes for:

Enabling lazy loading for gridfields
Deal with Groups, DBForeignKeys
Potentially optimize paginator

beerbohmdo · 2025-03-24T10:46:22Z

(and if anything, I don't think having multiple dropdown_field_threshold settings is a good thing)

I used the config from DBForeignKey before, but @GuySartorelli marked this as unrelated.

If we can handle this more generic I am happy. I have many large tables in my database.

beerbohmdo mentioned this pull request Jan 27, 2025

Group tree generation timeouts when you have a massive amount of groups #11577

Open

4 tasks

beerbohmdo force-pushed the 11577-slow-group-tree branch from 120baae to c8b41b0 Compare January 27, 2025 10:17

beerbohmdo changed the title ~~11577 Fix performance issues with large list of groups~~ FIX 11577 performance issues with large list of groups Jan 30, 2025

GuySartorelli requested changes Feb 3, 2025

View reviewed changes

beerbohmdo force-pushed the 11577-slow-group-tree branch 2 times, most recently from 7ffbd21 to 99aaf6b Compare February 5, 2025 07:58

FIX 11577 Performance issues with large list of groups

9ec4dd6

beerbohmdo force-pushed the 11577-slow-group-tree branch from 99aaf6b to 9ec4dd6 Compare February 5, 2025 08:09

beerbohmdo requested a review from GuySartorelli February 18, 2025 07:59

		$threshold = DBForeignKey::config()->get('dropdown_field_threshold');
		$overThreshold = $groups->count() > $threshold;

Conversation

beerbohmdo commented Jan 27, 2025 • edited by GuySartorelli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Manual testing steps

Issues

Pull request checklist

Uh oh!

GuySartorelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beerbohmdo commented Feb 5, 2025

Uh oh!

GuySartorelli commented Feb 18, 2025

Uh oh!

lekoala commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beerbohmdo commented Mar 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

beerbohmdo commented Jan 27, 2025 •

edited by GuySartorelli

Loading

lekoala commented Mar 24, 2025 •

edited

Loading