Skip to content

Comments

Slug generation performance improvement#118

Merged
LukeTowers merged 1 commit intowintercms:developfrom
RomainMazB:patch-2
Sep 28, 2022
Merged

Slug generation performance improvement#118
LukeTowers merged 1 commit intowintercms:developfrom
RomainMazB:patch-2

Conversation

@RomainMazB
Copy link
Contributor

The EXISTS sql statement will return true at the first existing value where the COUNT will uselessly continue to loop through the whole table which is inaccurate for this use case.

The syntax is more readable too IMO.

The `EXISTS` sql statement will return true at the first existing value where the `COUNT` will uselessly continue to loop through the whole table which is inaccurate for this use case.

The syntax is more readable too IMO.
RomainMazB added a commit to RomainMazB/storm that referenced this pull request Sep 27, 2022
@bennothommo
Copy link
Member

bennothommo commented Sep 27, 2022

I've given this a test by running the following in the Sluggable tests:

public function testSlugGenerationLargeDataset()
{
    // Create 2500 records
    for ($i = 0; $i < 2500; ++$i) {
        TestModelSluggable::create([
            'name' => 'test row ' . $i,
        ]);
    }

    $testModel1 = TestModelSluggable::create(['name' => 'test']);

    // Create another 2500 records
    for ($i = 2500; $i < 5000; ++$i) {
        TestModelSluggable::create([
            'name' => 'test row ' . $i,
        ]);
    }

    DB::listen(function ($query) {
        print_r([
            'query' => $query->sql,
            'bindings' => $query->bindings,
            'time' => $query->time,
        ]);
    });

    $testModel2 = TestModelSluggable::create(['name' => 'test']);
}

I found a very, very, very small performance benefit for using exists() over count(). I mean incredibly minuscule - even with 5000 records. exists() ran in 0.05 seconds, count() ran in 0.06 seconds, but it fluctuated, so I assume there's rounding going on. This is with SQLite.

The logic is sound, so I'm not against merging it - it makes sense that we only care if a record exists with a given slug, we don't need to know how many records have the slug, but I just wanted to make it clear this is a micro-optimisation.

@LukeTowers LukeTowers merged commit 623845f into wintercms:develop Sep 28, 2022
@LukeTowers
Copy link
Member

While the gains are small, the change does take it from an issue that gets slower with scale and turns it into a consistent performance operation instead which is always better IMO.

@RomainMazB RomainMazB deleted the patch-2 branch September 28, 2022 10:30
@bennothommo bennothommo added this to the 1.2.1 milestone Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants