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.3] Optimize Arr methods #15278

Closed
wants to merge 1 commit into from
Closed

[5.3] Optimize Arr methods #15278

wants to merge 1 commit into from

Conversation

bepsvpt
Copy link
Contributor

@bepsvpt bepsvpt commented Sep 5, 2016

  • Arr::exists

Using isset to optimize the process, only if the value is null will use array_key_exists to determine the given key exists.

  • Arr::last

Same as #15213, prevent array copy.

  • Arr::forget, Arr::set

Use foreach to prevent multiple array_shift.

@@ -120,7 +120,7 @@ public static function exists($array, $key)
return $array->offsetExists($key);
}

return array_key_exists($key, $array);
return isset($array[$key]) || array_key_exists($key, $array);
Copy link
Member

Choose a reason for hiding this comment

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

array_key_exists is pretty much the same speed on php 7, so you've actually made this slower

Copy link
Contributor

Choose a reason for hiding this comment

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

…and even if it's slower, the difference is super micro and just not worth it if I may add. Unless, of course, you're calling this method repeatedly for like 10,000 times.

Copy link
Member

Choose a reason for hiding this comment

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

Even then, you're making this twice as slow for cases where it is there and is null.

Copy link
Contributor

@fernandobandeira fernandobandeira Sep 5, 2016

Choose a reason for hiding this comment

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

There's a benchmark example in php.net

Benchmark (100000 runs):
array_key_exists() : 205 ms
isset() : 35ms
isset() || array_key_exists() : 48ms

However if you are running this 100000 times i guess that 157ms is not your main concern.
So indeed it's not worth it.

Also this is an old benchmark so array_key_exists on PHP7 and 5.6 should be faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only test this on local machine, and adding isset is faster.

However, when using sandbox online, array_key_exists is faster.

Benchmark : http://sandbox.onlinephpfunctions.com/code/90069414dc1b46c666fbfd96e1b9415b9d151378

@GrahamCampbell GrahamCampbell changed the title Optimize Arr methods [5.3] Optimize Arr methods Sep 5, 2016
@taylorotwell
Copy link
Member

Holding off on this for now because there are no benchmarks to prove the optimizations are relevant.

@bepsvpt
Copy link
Contributor Author

bepsvpt commented Sep 5, 2016

Benchmarks:

Arr::forget: http://sandbox.onlinephpfunctions.com/code/161e998b5eadba07d9b76feb61c629e8142176d6

Arr::set: http://sandbox.onlinephpfunctions.com/code/324e9cdb0d9c6662ceb30a1db534fd1e0fd206e9

forget, set are original and _forget, _set are optimized

@taylorotwell , except the Arr::exists, should i create a new pr?

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.

5 participants