[5.6] Faster implementation for Collection::mapToDictionary

[Lapayo]

I just ran into huge performance problems when eager loading multiple thousand rows (~20k in my case).

I tracked it down to the Dictionary::mapToDictionary method, which somehow is very slow when dealing with this amount of data.

I rewrote the method without map-reduce and it is much faster now. Let me know what you think!

The following benchmark was taken on an i7-6700 machine. (Benchmark script attached).
benchmap.zip

time for 100 (current): 5.4161071777344E-5
time for 20k (current): 17.208889961243
time for 25k (current): 35.498338937759
time for (new) 100: 2.5948047637939E-5
time for 20k (new): 0.012021064758301
time for 25k (new): 0.016914844512939

[thecrypticace]

Building arrays with reduce is quadratic so this isn't surprising. 👍

I'd suggest using a foreach loop if performance is the ultimate goal here. On a million items it ranges between 4x to 15x faster.

In the typical case, using foreach in mapToDictionary on a million items runs in 0.8s–4.1s for me (in PHP 7.1 and 7.2) whereas using $collection->each runs in 12s–17s.

(All benchmarks on homestead resulted in < 1s for a million items for foreach. On macOS it's ~4s)

[mfn]

I'd suggest using a foreach loop if performance is the ultimate goal here.

Would like to see this too. @Lapayo can you benchmark with plain foreach?

[Lapayo]

I just did, its indeed a lot faster - depending on the use case (y)
If the plain version is more likely merged I can commit that one. I was not sure if ->each() might be preferred.

time for 20k (plain): 0.0091784229278564
time for 20k (->each): 0.011384341001511
time for 100k (plain): 0.05624255490303
time for 100k (->each): 0.1215734539032
benchplainvseach.zip

[mfn]

@Lapayo very much appreciated!

I can't comment on the likelyhood, I would definitely favor the plain foreach variant.

Collections are by definition "things of many" and thus almost any operation has to loop in some way and it's a hot code path: even the tiniest slow down accumulates on larger sets, as you've demonstrated.

[deleugpn]

I wouldn't go as far as say that the second level of improvement (plain foreach over each) would be that important. Imagine 100k items of an actual eloquent record, you'd need a lot of memory available in order to not get a memory exhaustion, which would lead to a chunk, instead.

[taylorotwell]

Used foreach. Thanks.

[mfn]

@deleugpn

I wouldn't go as far as say that the second level of improvement (plain foreach over each) would be that important. Imagine 100k items of an actual eloquent record, you'd need a lot of memory available in order to not get a memory exhaustion, which would lead to a chunk, instead.

I see were you coming from. But I think you've to think more outside that.

This is a PR for the base Support\Collection, not Eloquent\Collection. So 100k of non-Eloquent models may will make sense for some, too.
Sure, at some point if performance is such an issue, you'd probably forgo using such a wrapper class.

But my take is: having identifier a hot code path and found a solution which a) is 100% backwards compatible and b) gives so much better performance characteristics is nothing to disagree with; on the contrary.

[gabrielkoerich]

Why not 5.5?

[tnorthcutt]

+1, would love to see this merged into 5.5 as well.