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

[5.7] Improve eager loading performance on MySQL #26434

Merged
merged 1 commit into from
Nov 8, 2018
Merged

[5.7] Improve eager loading performance on MySQL #26434

merged 1 commit into from
Nov 8, 2018

Conversation

staudenmeir
Copy link
Contributor

There is an old bug in the PDO implementation for MySQL that slows down larges WHERE IN queries. When the query has a few thousand bindings, the time execution time of fetchAll() increases non-linearly.

In Laravel, this especially affects eager loading. For models with integer keys, we can fix this by using raw WHERE IN clauses. The new whereInRaw() method applies intval() to all values, so this is as secure as a prepared statement.

It's certainly not a very elegant solution but still worth considering for the core. Since most Laravel sites run on MySQL/MariaDB and most sites use eager loading with integer keys (my assumptions), this would improve the performance on a lot of sites – even if they aren't eager loading thousands of records at once. The individual query may only gain a few milliseconds, but over all sites combined, this adds up.

This PR shows a sample implementation for HasMany relationships. The same changes would apply to all the other relationships.

Fixes #26051.

@laurencei
Copy link
Contributor

Given this is a performance related PR - would be worth providing some performance comparisons to see if the changes actually make a meaningful difference?

@X-Coder264
Copy link
Contributor

You have a performance comparison posted in the issue ticket -> #26051 (comment)

@JayBizzle
Copy link
Contributor

JayBizzle commented Nov 8, 2018

Just as a quick headline statistic from the referenced issue (#26051), some examples show a performance improvement of over 90% in query time

@mfn
Copy link
Contributor

mfn commented Nov 8, 2018

I don't see anything mysql specific, is this change for all drivers then?

@staudenmeir
Copy link
Contributor Author

@mfn Yes, the change affects all drivers. We could make it MySQL-specific, but I don't think that makes sense. All databases benefit from a raw query in this case, even if the performance improvement is smaller than on MySQL.

@taylorotwell taylorotwell merged commit a4405e9 into laravel:5.7 Nov 8, 2018
@mfn
Copy link
Contributor

mfn commented Nov 8, 2018

All databases benefit from a raw query in this case, even if the performance improvement is smaller than on MySQL.

Awesome, and well, great to see it's already merged; thx!

@staudenmeir staudenmeir deleted the where-in-raw branch November 8, 2018 14:53
@staudenmeir staudenmeir changed the title [5.7][WIP] Improve eager loading performance on MySQL [5.7] Improve eager loading performance on MySQL Nov 9, 2018
@vlakoff
Copy link
Contributor

vlakoff commented Nov 9, 2018

Do we really need the array_map('intval', $values)?

  • I guess it brings a performance hit
  • What if someone tries to use $builder->whereInRaw() with something other than ints? The cast seems really unexpected in this scenario ("raw" means "raw" after all)

@vlakoff
Copy link
Contributor

vlakoff commented Nov 9, 2018

I'd think of replacing $builder->whereInRaw with 2 methods: whereInRawInts and whereInRawStrings

  • whereInRawInts: expects to be provided with ints
  • whereInRawStrings: escapes the values: encloses with quotes, escapes the quotes, etc.


(edit: or alternative names, whereInInts and whereInStrings?)

@staudenmeir
Copy link
Contributor Author

staudenmeir commented Nov 9, 2018

@vlakoff The purpose of array_map('intval', $values) is to prevent people from using whereInRaw() with non-integer values. Raw queries like this are only secure with integer (and float) values. For everything else, we have to use bindings.

@vlakoff
Copy link
Contributor

vlakoff commented Nov 10, 2018

Ok, let's just forget about my whereInRawStrings (no bindings, "manual" escaping), as it hasn't been requested for yet, and outside the scope of the current case.

Still, I'd suggest renaming whereInRaw to whereInRawInts, to avoid confusion.

I'd also suggest replacing the array_map('intval', $values) with the following, slightly faster (~10%):

foreach ($values as &$value) {
    $value = (int) $value;
}

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.

7 participants