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.4] Faster implementation for Arr::crossJoin() #19864

Merged
merged 3 commits into from
Jul 2, 2017
Merged

[5.4] Faster implementation for Arr::crossJoin() #19864

merged 3 commits into from
Jul 2, 2017

Conversation

vlakoff
Copy link
Contributor

@vlakoff vlakoff commented Jul 2, 2017

Follow-up to #19236. ping @JosephSilber

Benchmarks, for 1000 iterations:

Arr::crossJoin([1, 2], ['a', 'b'], ['I', 'II', 'III']);

current: 429 ms
new: 30 ms

Arr::crossJoin([1, 2], ['a', 'b', 'c']);

current: 205 ms
new: 18 ms

props Serg for this implementation. (note I removed the array_filter line)

Also added a few tests to reinforce BC.

@vlakoff
Copy link
Contributor Author

vlakoff commented Jul 2, 2017

Changed tests to assertSame for strict array comparisons. This implementation's results have the same internal ordering as previous code.

@taylorotwell taylorotwell merged commit 73f8da3 into laravel:5.4 Jul 2, 2017
@thecrypticace
Copy link
Contributor

thecrypticace commented Jul 3, 2017

👍

Your reminder that building arrays with reduce, while fun, is accidentally quadratic.
https://twitter.com/AirspeedSwift/status/627469657264553984

(This is about Swift but the same concept applies here as well)

@vlakoff
Copy link
Contributor Author

vlakoff commented Jul 3, 2017

I just ran some quick'n'dirty tests, this new implementation seems to be O(n), and it seems the previous implementation was also O(n).

So the new implementation is just constantly faster than the previous one :)

@vlakoff vlakoff deleted the crossJoin branch July 3, 2017 19:34
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.

3 participants