is_array is expensive. avoid it in unpack_options() when possible.

Thanks for the patch, I tried a similar approach and saw the following in xhprof:

before patch
is_array 79,973 calls, 554,405 incl wall time

after patch
s_array 44,933 calls, 175,630 incl wall time

in your patch you do replace

        if (!isset($storage[$key]) || !is_array($storage[$key])) {

with

        if (empty($storage[$key])) {

I'm not sure you can do that, because empty will trigger a php notice if the variable you are testing is not set.
I modified your approach and did

        if (!isset($storage[$key]) || empty($storage[$key])) {

The user comments at http://us.php.net/manual/en/function.is-array.php#98156 suggest an even faster approach to testing if something is an array. I'm going to give it a try and will report back.

empty() will not trigger a notice if it is not set. empty() and isset() are the two notice-catching functions.

casting to an array as php.net is a lot more efficient, especially when you have a lot of views. For me, unpack_options was two times faster with this patch.
With attached patch, calls to is_array are 1/5 what they were before the patch.

Profiled with xhprof


BEFORE:
unpack_options #calls = 472 incl. cpu time = 633,458(16.7%)
is_array #calls = 79,973 incl. cpu time = 252,320 (6.2%)
AFTER:
unpack_options #calls = 473 incl. cpu time = 266,282 (7.8%)
is_array #calls = 15,983 incl. cpu time = 96,769 (1.4%)


Attached is a reworked patch.

Wow. That's shocking. I'm appalled that such a simple primitive is so slow and that using casting and !== is actually faster.

same here, but its been true for as long as I've done PHP, started in 2000. We had an in house cms at a previous job that used is_array and in_array extensively, switching to empty and using isset($var[$key]) gave us startling performance boosts. Do you want me to remove the unnecessary isset from that patch?

I'm a bit concerned that hunk 2 changes the logic. Right now, an empty array still triggers the clause, and we'll need to carefully study the code to see if that will cause issues with actual empty arrays. Table style options can have empty arrays so that's a good one to test on.

This can't be true.

I was seemingly able to reproduce your results for smaller arrays -- but then i realized that I had xdebug trace on. So I bet it's the benchmarking that adds so much to a function call that it's too slow.

This patch shouldn't change the logic that was introduced in the original patch's use of empty(). This patch just replaces is_array with the cast to an array tweak.

Performance is still better:


BEFORE:
unpack_options #calls = 472 incl. cpu time = 633,458(16.7%)
is_array #calls = 79,973 incl. cpu time = 252,320 (6.2%)
AFTER:
unpack_options #calls = 473 incl. cpu time = 247,482 (7.1%)
is_array #calls = 16,058 incl. cpu time = 60,231 (1.7%)


Just for posterity, here's a much fleshed out microtime-based benchmark.

chx's test suggests that the additional time spent is from profiling adding a bunch of overhead to the function call. It may be possible to do a reasonable test using timer_start and timer_stop in unpack_options and run it on a system without profiling.

So please re-benchmark without xhprof....

That's what I found too. I ran your benchmarks, and for large arrays, is_array is indeed faster. I found that when the array is 20 elements in length, the methods are equivalent. For arrays that are smaller, then the cast hack is faster.

There is an explanation of why over on stackoverflow (http://stackoverflow.com/questions/3470990/is-micro-optimization-worth-t...), the second answer deals with exactly this.

I guess views is generally using small arrays, hence the improvement in performance when profiling execution.

I think timer_start and timer_stop don't handle recursive functions very well. I'll try to find another way to profile. Did you see the other replies, using non-profiling micro benchmarks, casting to an array is quicker for small arrays.

Benchmarking without khprof enabled. Did ten runs for each option.

OK so that small arrays? Well running my benchmark above shows a surefire win for 5 or less and a maybe win for below 10 (on several runs I see a slower on the four comparisons here and there). 10 and above is a lose. Now is the question, will these arrays always be one digit large?

subscribing, it's definitely worth trying to figure this out, I come across D6 sites spending several hundred milliseconds in unpack_options() regularly.

I'd definitely be wary about profiling overhead, this is especially bad with xdebug, less so with xhprof, but it really matters if you are /only/ swapping a function call for something else.

Without profiling, what about using microtime just before code that call unpack options and then immediately after? That would give a more realistic data set. Either that or would need to var_export() a view then create a benchmark script using roughly the same data.

It would be great if Views got closer to being able to cache the result of unpack_options() between requests - so that the actual building of the view object doesn't have to run every request. Every time I've started to look at this that seemed like a very large task though, so potential quick wins like this are great.

#15 data used timer_start/stop around unpack options to measure.

omerida - and you had both the xhprof and xdebug extensions completely disabled for those?

subscribing

yes, no profiling going on.

I've got some bad news. I did some extensive benchmarking of this and there is no performance gain. The trick does seem to be xDebug itself. @omerida note that it is important to do the benchmarking without the xdebug PHP extension being loaded at all. Having profiling off is not enough.

Attached are my benchmarking results including a patch that shows how I did the benchmarking. (array) $options !== $options is actually about 15% slower than is_array().

These results actually open up a can of worms. We've done several micro-optimizations of core doing similar things over the past year or two that may have actually made things worse. We'll need to go back and re-assess all of those.

I'm going to mark this duplicate of #402944: Persistent caching for unpack_options() calls from building displays. While the other issue only adds caching, it does so in such a place that it's saving thousands of calls to unpack_options() on cache hits in the site I profiled.