Skip to content
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

ENH Do not use placeholders by default for foreignIDFilter() #10904

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Aug 7, 2023

Issue #10887

Performance tests:

10,000 records

I didn't test Member_GroupSet due to not wanting to truncate my Member and Group tables, though it's logical to expect that there would be performance gain since there was clearly a gain for all the others lists

HasManyList

placeholders on - avg 0.3304s
0.3144
0.3712
0.3279
0.3091
0.3295

placeholders off - avg 0.1584s
0.1558
0.1545
0.1551
0.1634
0.1632

ManyManyList

placeholders on - avg 0.3119s
0.3105
0.3118
0.3066
0.3130
0.3178

placeholders off - avg 0.1529s
0.1511
0.1478
0.1563
0.1565
0.1526

ManyManyThroughList

placeholders on - avg 0.8766s
0.9052
0.8893
0.8608
0.8687
0.8588

placeholders off - avg 0.6901s
0.6974
0.6993
0.6881
0.6811
0.6844

Test setup:

MyDataObject.php

<?php

use SilverStripe\ORM\DataObject;

class MyDataObject extends DataObject
{
    private static $table_name = 'MyDataObject';

    private static $db = [
        'Title' => 'Varchar'
    ];

    private static $has_many = [
        'MySubDataObjects' => MySubDataObject::class
    ];

    private static $many_many = [
        'MyManyDataObjects' => MyManyDataObject::class,
        'MyManyThroughDataObjects' => MyManyThroughDataObject::class
    ];
}

MySubDataObject.php

<?php

use SilverStripe\ORM\DataObject;

class MySubDataObject extends DataObject
{
    private static $table_name = 'MySubDataObject';

    private static $db = [
        'Title' => 'Varchar'
    ];

    private static $has_one = [
        'MyDataObject' => MyDataObject::class
    ];
}

MyManyDataObject.php

<?php

use SilverStripe\ORM\DataObject;

class MyManyDataObject extends DataObject
{
    private static $table_name = 'MyManyDataObject';

    private static $db = [
        'Title' => 'Varchar'
    ];

    private static $belongs_many_many = [
        'MyDataObject' => MyDataObject::class
    ];
}

MyManyThroughDataObject.php

<?php

use SilverStripe\ORM\DataObject;

class MyManyThroughDataObject extends DataObject
{
    private static $table_name = 'MyManyThroughDataObject';

    private static $db = [
        'Title' => 'Varchar'
    ];
}

MyThroughDataObject.php

<?php

use SilverStripe\ORM\DataObject;

class MyThroughDataObject extends DataObject
{
    private static $table_name = 'MyThroughDataObject';

    private static $has_one = [
        'MyDataObject' => MyDataObject::class,
        'MyManyThroughDataObject' => MyManyThroughDataObject::class,
    ];
}

PageController.php

<?php

use SilverStripe\Core\Config\Config;
use SilverStripe\Versioned\Versioned;
use SilverStripe\ORM\DB;
use SilverStripe\CMS\Controllers\ContentController;
use SilverStripe\ORM\DataList;

class PageController extends ContentController
{
    protected function init()
    {
        parent::init();
        //
        DB::query('truncate MyDataObject');
        DB::query('truncate MySubDataObject');
        DB::query('truncate MyManyDataObject');
        DB::query('truncate MyDataObject_MyManyDataObjects');
        DB::query('truncate MyManyThroughDataObject');
        DB::query('truncate MyThroughDataObject');
        $a = [];
        $b = [];
        $c = [];
        $d = [];
        $e = [];
        $f = [];
        for ($i = 1; $i <= 10000; $i++) {
            $a[] = "('Title $i')";
            $b[] = "('$i', 'Title $i')";
            $c[] = "('Title $i')";
            $d[] = "('$i', '$i')";
            $e[] = "('Title $i')";
            $f[] = "('$i', '$i')";
        }
        $as = implode(',', $a);
        $bs = implode(',', $b);
        $cs = implode(',', $c);
        $ds = implode(',', $d);
        $es = implode(',', $e);
        $fs = implode(',', $f);
        DB::query("insert into MyDataObject (Title) values $as");
        DB::query("insert into MySubDataObject (MyDataObjectID, Title) values $bs");
        DB::query("insert into MyManyDataObject (Title) values $cs");
        DB::query("insert into MyDataObject_MyManyDataObjects (MyDataObjectID, MyManyDataObjectID) values $ds");
        DB::query("insert into MyManyThroughDataObject (Title) values $es");
        DB::query("insert into MyThroughDataObject (MyDataObjectID, MyManyThroughDataObjectID) values $fs");
        //
        Versioned::set_reading_mode('Stage.Stage');
        $ids = MyDataObject::get()->column('ID');

        Config::modify()->set(DataList::class, 'use_placeholders_for_integer_ids', false);
        // $list = MyDataObject::get()->first()->MySubDataObjects();
        // $list = MyDataObject::get()->first()->MyManyDataObjects();
        $list = MyDataObject::get()->first()->MyManyThroughDataObjects();

        $s = microtime(true);
        $newList = $list->forForeignID($ids);
        $newList->toArray();
        $t = microtime(true) - $s;
        printf('%0.4f', $t);
        echo "<br>";
        $sql = $newList->dataQuery()->sql();
        echo $sql;
        die;
    }
}

@emteknetnz emteknetnz force-pushed the pulls/5/more-placeholders branch 5 times, most recently from 9b49c60 to db0533d Compare August 8, 2023 05:52
@emteknetnz emteknetnz marked this pull request as ready for review August 8, 2023 06:45
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a small change to make.

@emteknetnz emteknetnz force-pushed the pulls/5/more-placeholders branch from db0533d to 358cbc9 Compare August 8, 2023 22:46
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well locally.

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.

2 participants