[5.2] Validation performance optimisation #12651

themsaid · 2016-03-07T07:10:00Z

The use case for testing performance is that I'm uploading a large excel file, convert it to array, validate the array before saving into the database.

The array looks like:

array:7 [
  "products" => array:257 []
  "product-sizes" => array:257 []
  "product-size-ingredients" => array:1142 []
]

Inside the validator we flatten the data to the dot.format and iterate over the results in multiple areas in the code, like in the validateDistinct, validateArray, and initializeAttributeOnData.

The issue here is that we hit all the flattened records even the ones that don't belong to the attribute we're checking, and this is causing a huge performance issue.

This PR grabs only the required attribute values before flattening them with Arr::dot, so based on the example above if I'm validating products.code is distinct I'll only iterate over products only, not the whole data.

Sometimes the data is nested:

Validator::make(
    [
        'foo' => [
            'bar' => [['baz' =>1], ['baz' =>2]],
            'cc'
        ],
        'bb'
    ],
    ['foo.bar.*.baz' => 'distinct']
);

Here we only need to flatten $data['foo']['bar'] for the check, and Arr::only was only working for single dimensional arrays, so I modified Arr::only to accept dot-keys and used the explicit part of the attribute name foo.bar to get only this data before passing it to Arr:dot.

UPDATE:

The Arr::only method was left untouched, and a new method was added to the Validator extractData().

…a instead of all

…_only docblock

vlakoff · 2016-03-07T07:43:32Z

I'm not sure it's a safe idea to bring dot notation in Arr::only.

Currently Arr class makes use of data_*() helper only in Arr::pluck, and I think it would be better to not bring in more.
Somehow related: [5.2] Fix dot property access problem in Arr #12626.

To be more consistent your Arr::only should use Arr::get/has instead, but precisely because of the #12626 above I'm not sure that would work.

I think the best solution might be to not touch Arr::only and add a new protected method in Validator for your purpose.

vlakoff · 2016-03-07T07:48:18Z

src/Illuminate/Support/Arr.php

+        $attributeData = [];
+
+        foreach ((array) $keys as $key) {
+            if (! $keyValue = data_get($array, $key)) {


You miss all falsy values because of this.

hmmm, this one is to pass the following test:

$array = ['name' => 'taylor', 'age' => 26]; $this->assertEquals(['name' => 'taylor'], array_only($array, ['name'])); $this->assertSame([], array_only($array, ['nonExistingKey']));

Not sure if there are other cases I missed.

Ahaaa, you mean null, 0, false, etc..? You are right, this should be fixed.

themsaid · 2016-03-07T07:48:32Z

Well giving it a deep thought I find the new addition to array_only is quite useful, maybe the method itself can be left alone but it'd be nice to have another method that outputs an array of only the keys you need using dot notation.

I'm thinking of API response for example, maybe you want to present ['parent'] and ['other_parent']['child1'] only for example.

vlakoff · 2016-03-07T07:57:03Z

Indeed it would be useful, I only say I think this goes outside of the scope of Arr.

Just some idea for later, creating some new class Illuminate\Support\Data, and moving into it all the heavy, "swiss-knife" functions, namely data_get/set() to begin with. These functions are a fundamental part of the framework and are presented as simple helpers, it looks a bit wrong to me.

themsaid · 2016-03-07T08:12:33Z

I see, maybe a Data::only() method in that class instead of Arr::only(). Let's see what Taylor thinks of the matter.

vlakoff · 2016-03-07T08:52:39Z

src/Illuminate/Validation/Validator.php

+     */
+    protected function getExplicitAddress($attribute)
+    {
+        return str_replace_last('.', '', explode('*', $attribute)[0]);


Should use the Str::replaceLast method.

Or maybe just rtrim( ... , '.') ?

themsaid · 2016-03-07T09:18:34Z

@vlakoff updates were made. Thanks for the notes.

mark86092 · 2016-03-07T14:59:47Z

src/Illuminate/Support/Arr.php

+                continue;
+            }
+
+            data_set($attributeData, $key, $keyValue);


Do your $key contain "*" ? I think the data_set is too "useful", Arr::set should work.

@vlakoff Yes, the Arr::set works on dots

Ah, right. So you could use Arr::get/set in this modified Arr::only. It would be a much better fit, while still working with the code in Validator.

This modifed Arr::only is probably slower, that's the only caveat I can think of.

@vlakoff, I'm even on data_get or Arr::get/set, since I agree on your concerns above due to #12626.
for data_set part, what I think is use simpler code as possible.

vlakoff · 2016-03-07T15:05:24Z

Because of the getExplicitAddress's just above in Validator, there could not be *, but there could be dots.

taylorotwell · 2016-03-28T15:21:03Z

Would it be possible for people to bypass validation by passing missing as their value? Could that possibly be exploited in any way?

taylorotwell · 2016-03-28T15:39:45Z

What did the before / after benchmarks look like on this?

themsaid · 2016-03-28T15:44:27Z

@taylorotwell If the value passed was __missing__ the end result of extractData() will be an empty array. I believe an empty array of data is not harmful, maybe we should leave the value as null instead of __missing__.

themsaid · 2016-03-28T16:26:39Z

@taylorotwell doing a very basic benchmarking, applying the distinct rule on 1,656 array items exceeded 30 seconds of execution. Applying the change and only passing the data the rule needs dropped it to 8 seconds.

The test is on an array that looks like:

array:7 [
  "products" => array:257 []
  "product-sizes" => array:257 []
  "product-size-ingredients" => array:1142 []
]

The rules are:

[
    'products.*.id' => 'required|distinct',
    'product-sizes.*.id' => 'required|distinct',
]

The difference in execution time is that before we pass all the data to the distinct rule to check for only the ones we need:

 $data = Arr::where(Arr::dot($this->data), function ($key) use ($attribute, $attributeName) {
            return $key != $attribute && Str::is($attributeName, $key);
        });

The change is:

$attributeData = $this->extractData($explicitAddress);
        $data = Arr::where(Arr::dot($attributeData), function ($key) use ($attribute, $attributeName) {
            return $key != $attribute && Str::is($attributeName, $key);
        });

Here extractData will only bring products when validating for products, and product-sizes when checking for product sizes. product-size-ingredients will never be passed to the validator method because we don't need it.

vlakoff · 2016-03-28T20:56:27Z

Just as a suggestion, how about this:

protected function extractData($attribute)
{
    static $missing;
    $missing || $missing = random_bytes(64);

    $results = [];

    $value = Arr::get($this->data, $attribute, $missing);

    if ($value !== $missing) {
        Arr::set($results, $attribute, $value);
    }

    return $results;
}

Closes #12651 Signed-off-by: Graham Campbell <graham@alt-three.com>

…ation-performance

themsaid added 3 commits March 7, 2016 06:35

optimise distinct for performance

85a1e94

modify array_only and use it to get relevant validation attribute dat…

766d474

…a instead of all

extract method to get explicit address in Validator and update array…

e1388b3

…_only docblock

vlakoff reviewed Mar 7, 2016
View reviewed changes

consider falsy values in array_only

bdd1e2d

vlakoff reviewed Mar 7, 2016
View reviewed changes

themsaid added 2 commits March 7, 2016 09:17

refine getExplicitAddress and add tests

d42484a

fix cs

538fc65

mark86092 reviewed Mar 7, 2016
View reviewed changes

use Arr methods in the new Arr::only

9244f6d

mark86092 mentioned this pull request Mar 8, 2016

Return only entered items in Request::only() #12668

Closed

themsaid added 2 commits March 18, 2016 14:39

include a in-class method instead of updating Arr::only

9a9bc69

update extractData method and its docblock

ffca0e9

themsaid changed the title ~~[5.2] Validation performance optimisation~~ [5.2] [WIP] Validation performance optimisation Mar 28, 2016

themsaid changed the title ~~[5.2] [WIP] Validation performance optimisation~~ [5.2] Validation performance optimisation Mar 28, 2016

GrahamCampbell pushed a commit that referenced this pull request Apr 1, 2016

Validation performance optimization

7e8cb0d

Closes #12651 Signed-off-by: Graham Campbell <graham@alt-three.com>

themsaid added 2 commits April 1, 2016 15:56

Merge branch '5.2' of https://github.com/laravel/framework into valid…

84bcf4d

…ation-performance

handle a missing case

32b9eb8

taylorotwell merged commit 32b9eb8 into laravel:5.2 Apr 1, 2016

themsaid deleted the validation-performance branch April 1, 2016 20:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5.2] Validation performance optimisation #12651

[5.2] Validation performance optimisation #12651

themsaid commented Mar 7, 2016

vlakoff commented Mar 7, 2016

vlakoff Mar 7, 2016

themsaid Mar 7, 2016

themsaid Mar 7, 2016

themsaid commented Mar 7, 2016

vlakoff commented Mar 7, 2016

themsaid commented Mar 7, 2016

vlakoff Mar 7, 2016

vlakoff Mar 7, 2016

themsaid commented Mar 7, 2016

mark86092 Mar 7, 2016

mark86092 Mar 7, 2016

vlakoff Mar 7, 2016

mark86092 Mar 7, 2016

vlakoff commented Mar 7, 2016

taylorotwell commented Mar 28, 2016

taylorotwell commented Mar 28, 2016

themsaid commented Mar 28, 2016

themsaid commented Mar 28, 2016

vlakoff commented Mar 28, 2016

[5.2] Validation performance optimisation #12651

[5.2] Validation performance optimisation #12651

Conversation

themsaid commented Mar 7, 2016

UPDATE:

vlakoff commented Mar 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

themsaid commented Mar 7, 2016

vlakoff commented Mar 7, 2016

themsaid commented Mar 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

themsaid commented Mar 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlakoff commented Mar 7, 2016

taylorotwell commented Mar 28, 2016

taylorotwell commented Mar 28, 2016

themsaid commented Mar 28, 2016

themsaid commented Mar 28, 2016

vlakoff commented Mar 28, 2016