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

NEW Use custom list for eagerloaded relations #10869

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Jul 11, 2023

Performance comparisons

The following comparison table was made using this code, where Player and Team are simple DataObject models, and Team has a many_many relation to Player.
The database has 29 Team records, each of which has 10 Player records.

$eagerLoad = true;

$teams = Team::get();
if ($eagerLoad) {
    $teams = $teams->eagerLoad('Players');
}

memory_reset_peak_usage();

foreach ($teams as $team) {
    foreach ($team->Players() as $player) {
        echo ''; // effectively no-op
    }
}

var_dump(memory_get_peak_usage() / 1024 / 1024); // memory usage in MB

The times were recorded from the lekoala debugbar, which shows the total ms for db calls in the request. It necessarily includes some unrelated queries, but those queries are fixed so they don't affect the difference between implementations.

To get the values, I ran the above code 3 times for each scenario, and the table reflects the averages of the values.

total db query time memory usage
no eager loading 16.21ms 0.96MB
eager loading before 7.95ms 1.44MB
eager loading after 7.69ms 1.23MB

Issue

Out of scope

@GuySartorelli GuySartorelli marked this pull request as draft July 11, 2023 05:37
@GuySartorelli GuySartorelli force-pushed the pulls/5/new-eagerloaded-relationlist branch 4 times, most recently from 7bbe146 to efe7852 Compare July 19, 2023 03:32
@GuySartorelli GuySartorelli force-pushed the pulls/5/new-eagerloaded-relationlist branch 6 times, most recently from 1fffd4f to 51bfa5e Compare July 24, 2023 00:50
@GuySartorelli GuySartorelli marked this pull request as ready for review July 24, 2023 01:15
@GuySartorelli GuySartorelli force-pushed the pulls/5/new-eagerloaded-relationlist branch 4 times, most recently from 58c7f1c to 56d6bb4 Compare July 24, 2023 01:47
src/ORM/Connect/MySQLQuery.php Show resolved Hide resolved
src/ORM/DataList.php Show resolved Hide resolved
src/ORM/DataList.php Show resolved Hide resolved
src/ORM/DataList.php Show resolved Hide resolved
src/ORM/DataList.php Show resolved Hide resolved
src/ORM/EagerLoadedDataList.php Outdated Show resolved Hide resolved
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

There's a lot of changes here so I'll probably need to do 2 or 3 rounds of review

On extra thing to add / add a test for is that when attempting to filter by something with a dot e.g. ->filter(['MyRelation.Title' => 'lorem']) - we should be throwing an exception so that the developer is aware this isn't possible

We currently have this in the corresponding docs pr
"
Note also that EagerLoadedDataList can't currently filter or sort by fields on relations using dot syntax (e.g. sort('MySubRelation.Title') won't work), nor filter using SearchFilter syntax (e.g. filter('Title:StartsWith', 'Prefix')).
"

src/ORM/EagerLoadedDataList.php Outdated Show resolved Hide resolved
src/ORM/EagerLoadedDataList.php Outdated Show resolved Hide resolved
src/ORM/EagerLoadedDataList.php Outdated Show resolved Hide resolved
src/ORM/EagerLoadedDataList.php Outdated Show resolved Hide resolved
src/ORM/EagerLoadedDataList.php Outdated Show resolved Hide resolved
tests/php/ORM/ArrayLibTest.php Outdated Show resolved Hide resolved
tests/php/ORM/DataListEagerLoadingTest.php Outdated Show resolved Hide resolved
src/ORM/ArrayLib.php Outdated Show resolved Hide resolved
@GuySartorelli GuySartorelli force-pushed the pulls/5/new-eagerloaded-relationlist branch 2 times, most recently from ae2857d to 90c4ba3 Compare July 31, 2023 02:12
@GuySartorelli
Copy link
Member Author

On extra thing to add / add a test for is that when attempting to filter by something with a dot e.g. ->filter(['MyRelation.Title' => 'lorem']) - we should be throwing an exception so that the developer is aware this isn't possible

Test added.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Looks like there's some missing unit tests in EagerLoadedListTest.php

EagerLoadedList public methods:

Has dedicated unit test

  • dataClass
  • getIterator
  • toNestedArray
  • map
  • each
  • max
  • min
  • avg
  • sum
  • canFilterBy
  • canSortBy
  • find
  • filter
  • filterAny
  • exclude
  • excludeAny
  • addFilter
  • subtract
  • filterByCallback
  • byID
  • byIDs
  • sort
  • shuffle
  • reverse
  • limit
  • has

Tested in other tests

  • offsetGet
  • add
  • toArray
  • first
  • last
  • column
  • count

Not tested

  • dbObject
  • getIDList
  • setByIDList
  • forForeignID
  • getRows
  • remove
  • columnUnique
  • debug
  • offsetExists
  • relation
  • createDataObject
  • getExtraData
  • getExtraFields
  • exists
  • offsetSet
  • offsetUnset

@GuySartorelli GuySartorelli force-pushed the pulls/5/new-eagerloaded-relationlist branch from 90c4ba3 to 958c133 Compare August 1, 2023 01:10
@GuySartorelli
Copy link
Member Author

I've now added tests for those.
For the ones that are implemented (i.e. don't just throw an exception) in the new list, I've added the test for both the new list and for DataList (or the relevant subclass) which was also missing this coverage.

I've changed the implementation of remove() to more closely match DataList's API, i.e. you pass in a DataObject record now just like you do with DataList.

I've changed the API for add() to match that of DataList - i.e. it expects a DataObject record - but it throws an exception, because there's no clean way to get all of the data out into an array format to be stored correctly as 'rows' in this list type.
The old add() implementation has been moved to addRow().

I've also improved the relation() implementation as I noticed while testing that there will be cases where we already have that data and therefore don't need to trigger a new db call.

Finally, I've swapped the order of arguments in the constructor - it is more intuitive to pass the dataclass first, I've found. This also means we can easily give a default for $dataListClass in the future if we want to.

@GuySartorelli GuySartorelli force-pushed the pulls/5/new-eagerloaded-relationlist branch from 958c133 to ea68fa6 Compare August 1, 2023 01:14
@GuySartorelli GuySartorelli force-pushed the pulls/5/new-eagerloaded-relationlist branch from ea68fa6 to 6397641 Compare August 1, 2023 01:45
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Main thing is strongly type all the things

src/ORM/EagerLoadedList.php Outdated Show resolved Hide resolved
src/ORM/EagerLoadedList.php Show resolved Hide resolved
src/ORM/EagerLoadedList.php Outdated Show resolved Hide resolved
src/ORM/EagerLoadedList.php Outdated Show resolved Hide resolved
src/ORM/EagerLoadedList.php Outdated Show resolved Hide resolved
src/ORM/EagerLoadedList.php Outdated Show resolved Hide resolved
src/ORM/EagerLoadedList.php Outdated Show resolved Hide resolved
@GuySartorelli GuySartorelli force-pushed the pulls/5/new-eagerloaded-relationlist branch from 6397641 to 5d0556d Compare August 3, 2023 02:29
@emteknetnz emteknetnz merged commit ae49e13 into silverstripe:5 Aug 3, 2023
12 checks passed
@emteknetnz emteknetnz deleted the pulls/5/new-eagerloaded-relationlist branch August 3, 2023 03:33
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