[5.6] Optimize query builder's pluck() method by franzliedke · Pull Request #23482 · laravel/framework

franzliedke · 2018-03-10T15:15:20Z

Proposed by @vlakoff in laravel/ideas#98 and yesterday acknowledged by Taylor...

This refactors the pluck method (and, to stay DRY, the get method) of Illuminate\Database\Query\Builder to no longer rely on object_get internally. As reported in the aforementioned issue, this can become a bottleneck when looping over a large number of results.

I've inlined Collection::pluck and then Arr::pluck, subsequently throwing away any overly generic code that is not necessary in this use case - because we know the data type of query results. Also, we don't need the other features of object_get, such as dot notation and default values.

franzliedke · 2018-03-10T15:19:18Z

~~Please note there are currently two failing tests.~~ This is because the test case sets up mocks as if the PDO fetch mode were still $PDO::FETCH_COLUMN. As far as I can tell, the fetch mode is now hard-coded to $PDO::FETCH_CLASS, so technically the test case could be considered incorrect.

What's the status of fetch code? It seems as if it cannot be changed in full-stack Laravel, whereas it can be changed when using illuminate/database as a stand-alone package (via Capsule).

So please tell me which is the correct way forward:

Change the tests to return stdClass objects.
Change pluck so that it can deal both with arrays and objects.

Even if the second option is preferred, taking care of this change in the tests may be worthwhile as well, as this would make the mocks more realistic (because full-stack Laravel always returns objects).

franzliedke · 2018-03-10T15:20:40Z

+     * @param  callable  $callback
+     * @return mixed
+     */
+    protected function onceWithColumns($columns, $callback)


Is this the correct location for the helper method? Or should it be placed below get()?

vlakoff · 2018-03-11T01:40:29Z

Thank you for your work on this. Much appreciated.

Ah ah yes, fetch mode has been removed, see notably #17557. Still, users can modify the fetch mode, so currently your code would break with "array fetch modes".

Off the top of my head:

Support object and array, but check has to be outside the loop. Then possibly 2 code branches, each with its loop.
Trim stuff such as "dot nested access" and "wildcards"

vlakoff · 2018-03-11T01:46:09Z

Also, keep in mind the PDO::FETCH_CLASS mode (but actually mapping to a user-defined class, not returning simple stdClass): laravel/ideas#98 (comment).

(as a reminder: at some time, laravel was using PDO::FETCH_CLASS to return stdClass instances. It works, but the proper pick should have been PDO::FETCH_OBJ. I've had fixed that since then.)

vlakoff · 2018-03-11T01:50:08Z

+        foreach ($queryResult as $row) {
+            $itemValue = $row->$column;
+
+            if (is_null($key)) {


I would move the is_null($key) function call outside of the loop.

(replacing with $key === null would be fine too, but you know the laravel convention...)

jmarcher · 2018-03-12T13:45:54Z

Can you provide some benchmarks? I believe this kind of changes does not get merged without some kind of benchmarks.

franzliedke · 2018-03-12T14:20:41Z

I will gladly do that once I get some official buy-in. ;)

franzliedke · 2018-03-16T22:54:20Z

Okay, I refactored a bit, tests are working.

Benchmark

Table with 10885 rows, 1000 iterations
Old method: 21.29s
New method: 11.56s

So, almost 2x speedup.

vlakoff · 2018-03-18T09:02:17Z

+            } else {
+                $results[$row->$key] = $row->$column;
+            }
+        }


I would move the is_null() function call outside of the loop:

if (is_null($key)) { foreach ($queryResult as $row) { $results[] = $row->$column; } } else { foreach ($queryResult as $row) { $results[$row->$key] = $row->$column; } }

It's a bit longer, but saving thousands of function calls is worth it, and the code is still readable, maybe even a bit better.

vlakoff · 2018-03-18T09:07:20Z

+            } else {
+                $results[$row[$key]] = $row[$column];
+            }
+        }


Same is_null() unlooping as in pluckFromObjectColumn().

vlakoff · 2018-03-18T09:18:57Z

+
+        return tap($callback(), function () use ($original) {
+            $this->columns = $original;
+        });


I know this one is "Laravel style", but sorry it's just incredibly painful to understand, especially if you are not used to this voodoo helper. Can't we just stay with the regular and simple lines:

$results = $callback(); $this->columns = $original; return $results;

Yeah, you're right. (I'm used to it from Ruby land, where it's far more readable.) I've advocated against the overuse of tap in Laravel in the past myself, whoops. Thanks for the feedback.

vlakoff · 2018-03-18T09:26:35Z

Strong +1 for this.

Sure there is a bit of added code, but it's not that much, and it's still very understandable. The DB pluck() method is noticeably slow, and I think such a performance boost is clearly worth it.

Also, would you rerun your benchmark with the unlooped is_null() calls? To prove the benefit ;)

This allows us to reimplement pluck() without calling get(), while avoiding the duplication of a lot of code. As a next step, pluck() can be re-implemented without relying on the (comparatively expensive) same method on the Collection class.

This prepares for the final step of removing (without replacement) the expensive parts of its implementation (from which, I suspect, the code will get shorter again).

Because we don't need all the features of data_get() - such as dot access - there is no need to use the function, which can become expensive when doing this with many, many rows in a loop.

franzliedke · 2018-03-18T12:20:45Z

@vlakoff I've taken care of your feedback, thank you!

Updated Benchmark

Table with 10885 rows, 1000 iterations
Old method: 22.13s
New method: 10.71s

carusogabriel · 2018-03-18T12:54:29Z

@franzliedke @vlakoff $foo === null is faster, and safer, IIRC than is_null($foo) :)

vlakoff · 2018-03-18T12:55:06Z

Comparing this bench with the previous one, the is_null() unlooping increased the speedup from 1.84x to 2.07x. Which is good to take, let's optimize completely.

vlakoff · 2018-03-18T12:59:10Z

@carusogabriel Indeed. If it were up to me, I would definitely use this. But the Laravel codebase uses is_null() everywhere.

franzliedke · 2018-03-18T13:03:39Z

Well, since we're not doing it in a loop, this type of micro-optimization won't have any measurable effect anyway.

carusogabriel · 2018-03-18T13:07:38Z

@vlakoff Maybe is some topic we can put at laravel/ideas.

@franzliedke Isn't micro, we reduce from 3 opcodes to 1, see:

is_null: https://3v4l.org/IZY4D/vld#output
=== null: https://3v4l.org/pPc5k/vld#output

thecrypticace · 2018-03-18T15:59:44Z

The difference in number of opcodes is because of (what I think is called) constant folding. An optimization that is performed by PHP because null === null is always true.

is_null(foo()) gives similar output to foo() === null. The difference being IS_IDENTICAL versus TYPE_CHECK opcodes. It's highly unlikely that this difference matters at all.

mfn · 2018-03-19T14:34:41Z

Totally like the PR.

But shouldn't we be cautious and rather target master/5.7 with this?

JosephSilber · 2018-03-25T03:26:12Z

+
+        $result = $callback();
+
+        $this->columns = $original;


Shouldn't this be wrapped in a try/finally block? We want to make sure we always reset them.

It wasn't before my refactoring, so I don't want to change this here...

vlakoff · 2018-04-04T14:21:03Z

So… it seems to be ready. Anything remaining to do?

vlakoff · 2018-04-05T17:48:22Z

Replying to mfn:

But shouldn't we be cautious and rather target master/5.7 with this?

The change is supposed to be transparent ;-)

If ever it breaks something, postponing the change to 5.7 wouldn't have made it better. In such a scenario, we would have to fix it asap or revert the change if not quickly fixable. Would be the same for any branch.

edit: Applying it to 5.7, it would have some "beta testing" phase, indeed. But as it's not changing the API, I still think it should be applied the soonest (and fixed the soonest, in worst case scenario).

jaketoolson · 2018-05-03T05:29:20Z

Optimizing pluck (on our own fork) on a project resulted in a previous ~2-minute process optimizing to around 20 seconds. Glad to see someone has also encountered this as well and raised attention to it.

taylorotwell · 2018-05-23T13:21:53Z

Thanks. Sorry for the delay 😄

franzliedke mentioned this pull request Mar 10, 2018

pluck() performance issues (specifically Query\Builder::pluck) laravel/ideas#98

Closed

franzliedke commented Mar 10, 2018

View reviewed changes

vlakoff reviewed Mar 11, 2018

View reviewed changes

franzliedke changed the title ~~[5.6] Optimize query builder's pluck() ,method~~ [5.6] Optimize query builder's pluck() method Mar 16, 2018

franzliedke force-pushed the 5.6-optimize-query-pluck branch 2 times, most recently from 07201ac to f6894c0 Compare March 16, 2018 22:52

vlakoff reviewed Mar 18, 2018

View reviewed changes

franzliedke added 4 commits March 18, 2018 13:18

Extract onceWithColumn() method

7e5ef3f

This allows us to reimplement pluck() without calling get(), while avoiding the duplication of a lot of code. As a next step, pluck() can be re-implemented without relying on the (comparatively expensive) same method on the Collection class.

Inline Array::pluck method

f4b07f6

This prepares for the final step of removing (without replacement) the expensive parts of its implementation (from which, I suspect, the code will get shorter again).

Optimize query builder's pluck implementation

a1d991b

Because we don't need all the features of data_get() - such as dot access - there is no need to use the function, which can become expensive when doing this with many, many rows in a loop.

Rename test method

5e444b7

franzliedke force-pushed the 5.6-optimize-query-pluck branch from f6894c0 to 5e444b7 Compare March 18, 2018 12:18

laravel deleted a comment from vlakoff Mar 18, 2018

JosephSilber reviewed Mar 25, 2018

View reviewed changes

taylorotwell merged commit 5e444b7 into laravel:5.6 May 23, 2018

franzliedke deleted the 5.6-optimize-query-pluck branch May 23, 2018 17:52

Conversation

franzliedke commented Mar 10, 2018

Uh oh!

franzliedke commented Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlakoff commented Mar 11, 2018

Uh oh!

vlakoff commented Mar 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarcher commented Mar 12, 2018

Uh oh!

franzliedke commented Mar 12, 2018

Uh oh!

franzliedke commented Mar 16, 2018

Benchmark

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlakoff commented Mar 18, 2018

Uh oh!

franzliedke commented Mar 18, 2018

Updated Benchmark

Uh oh!

carusogabriel commented Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vlakoff commented Mar 18, 2018

Uh oh!

vlakoff commented Mar 18, 2018

Uh oh!

franzliedke commented Mar 18, 2018

Uh oh!

carusogabriel commented Mar 18, 2018

Uh oh!

thecrypticace commented Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfn commented Mar 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlakoff commented Apr 4, 2018

Uh oh!

vlakoff commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaketoolson commented May 3, 2018

Uh oh!

taylorotwell commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

franzliedke commented Mar 10, 2018 •

edited

Loading

carusogabriel commented Mar 18, 2018 •

edited

Loading

thecrypticace commented Mar 18, 2018 •

edited

Loading

vlakoff commented Apr 5, 2018 •

edited

Loading