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

DOC Document manipulating eager loading queries #456

Merged

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Feb 14, 2024

Description

Documents silverstripe/silverstripe-framework#11140

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no TODO comments, unrelated rewording/restructuring, or arbitrary changes)
    • Small amounts of additional changes 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
  • The changes follow our writing style guide
  • Code examples follow our coding conventions
  • CI is green

@GuySartorelli GuySartorelli marked this pull request as draft February 14, 2024 02:58
@GuySartorelli GuySartorelli force-pushed the pulls/5/eagerload-filtering branch 3 times, most recently from 79359bc to 75fe8ee Compare February 14, 2024 21:23
@GuySartorelli GuySartorelli marked this pull request as ready for review February 14, 2024 21:23
@GuySartorelli GuySartorelli force-pushed the pulls/5/eagerload-filtering branch 4 times, most recently from 5be588a to c17b502 Compare February 19, 2024 03:38
]);
```

The list passed into your callback represents the query for that relation on *all* of the records you're fetching. For example, above the `$list` variable is a `DataList` that will fetch all `Player` records in the `Players` relation for all `Team` records (so long as they match the filter applied in the callback).
Copy link
Member

Choose a reason for hiding this comment

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

"above the $list variable" sounds really weird, should this be something like "before the $list DataList is created another DataList containing ... Players ..." ?

Copy link
Member Author

@GuySartorelli GuySartorelli Feb 19, 2024

Choose a reason for hiding this comment

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

Swapped to the `$list` variable above. All this part is saying is that the $list variable in the example is a DataList, and what that list contains.

use SilverStripe\ORM\DataList;

$teams = Team::get()->eagerLoad([
// Remove the callback that was previously defined
Copy link
Member

Choose a reason for hiding this comment

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

Previously defined where? In the different example above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously defined anywhere at any time before this. Yes in the example above, but also in general, if there was previously a callback defined, setting this to null here will remove it.

en/08_Changelogs/5.2.0.md Outdated Show resolved Hide resolved
Comment on lines 82 to 84
- HasMany - ~29% faster (0.1080s vs 0.7358s)
- ManyMany - ~21% faster (0.1264s vs 0.9002s)
- ManyManyThrough - ~21% faster (0.2511s vs 1.0719s)
Copy link
Member

@emteknetnz emteknetnz Feb 19, 2024

Choose a reason for hiding this comment

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

See #458 (review) you need to "invert" these numbers

e.g. 0.2511s vs 1.0719s is ~400% faster, rather than 21%

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't get it, but if you just feed me the calculation you want me to use I'll use it.

Copy link
Member

Choose a reason for hiding this comment

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

(1.0719 / 0.2511) * 100 = ~427% faster

Copy link
Member Author

@GuySartorelli GuySartorelli Feb 19, 2024

Choose a reason for hiding this comment

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

We ended up with (OLD / NEW * 100) - 100 as the calculation.
The rationale is: If old is 300 and new is 200, the new one is 50% faster than the old one.
I'll open a PR to update the 5.1 changelog as well. Double checked - the 5.1 changelog already uses the updated calculation.

Comment on lines 787 to 790
> [!CAUTION]
> Filtering an `EagerLoadedlist` is significantly (up to 85%) slower than performing that manipulation on the eager loading query.
>
> See [manipulating eagerloading queries](#manipulating-eager-loading-queries).
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this here as an early warning about this, for people who don't read the whole docs (i.e. most everyone).
It is intentionally duplicating a warning in that section.

use SilverStripe\ORM\DataList;

$teams = Team::get()->eagerLoad([
// Remove the callback that was previously defined
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Remove the callback that was previously defined
// Remove any callback that was previously defined

Copy link
Member Author

Choose a reason for hiding this comment

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

But it's not any callback, it's the callback for this relation chain. I'll try to make that clearer.

Also can you please try to keep changes like this which are related to discussions inside the threads which have the discussion? i.e. this has context in #456 (comment) which it's now decoupled from.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Comment on lines 82 to 84
- HasMany - ~29% faster (0.1080s vs 0.7358s)
- ManyMany - ~21% faster (0.1264s vs 0.9002s)
- ManyManyThrough - ~21% faster (0.2511s vs 1.0719s)
Copy link
Member

Choose a reason for hiding this comment

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

(1.0719 / 0.2511) * 100 = ~427% faster

@@ -784,6 +784,11 @@ The N + 1 query problem can be alleviated using eager loading which in this exam
$teams = Team::get()->eagerLoad('Players');
```

> [!CAUTION]
> Manipulating the eager loading query is significantly (up to ~600%) faster than Filtering an `EagerLoadedlist` after the query has been made.
Copy link
Member Author

Choose a reason for hiding this comment

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

This percentage is based on the middle test in the issue - i.e. used to be 0.9002s, now 0.1264s. Rounded to the nearest 100

@emteknetnz emteknetnz merged commit aa9ef53 into silverstripe:5 Feb 20, 2024
3 checks passed
@emteknetnz emteknetnz deleted the pulls/5/eagerload-filtering branch February 20, 2024 03:17
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