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

Array optimizations #8048

Merged
merged 5 commits into from Aug 7, 2019

Conversation

@asterite
Copy link
Member

commented Aug 5, 2019

This PR optimizes a few array methods when dealing with small arrays (less than 16 elements). These optimizations are also done by Ruby. I tried using a bigger number (like 32): in some cases the optimized version was still faster than the non-optimized one, but in some other cases it was worse. With 16 it's always faster so it's probably a good number (and why Ruby chose it).

-

Code:

require "benchmark"
require "./old_array"

size = 16

old_a1 = OldArray.new(size) { Random::DEFAULT.hex(32) }
old_a2 = OldArray.new(size) { |i| old_a1[i] }

new_a1 = Array.new(size) { |i| old_a1[i] }
new_a2 = Array.new(size) { |i| old_a1[i] }

Benchmark.ips do |x|
  x.report("old") { old_a1 - old_a2 }
  x.report("new") { new_a1 - new_a2 }
end

Result:

old 784.20k (  1.28µs) (± 7.36%)   864B/op   4.31× slower
new   3.38M (295.74ns) (± 6.56%)  32.0B/op        fastest

|

Code:

size = 8

old_a1 = OldArray.new(size) { Random::DEFAULT.hex(32) }
old_a2 = OldArray.new(size) { |i| old_a1[i] }

new_a1 = Array.new(size) { |i| old_a1[i] }
new_a2 = Array.new(size) { |i| old_a1[i] }

Benchmark.ips do |x|
  x.report("old") { old_a1 | old_a2 }
  x.report("new") { new_a1 | new_a2 }
end

Result:

old   1.68M (593.85ns) (± 5.76%)  560B/op   1.92× slower
new   3.24M (308.61ns) (±10.18%)  240B/op        fastest

&

Code:

require "benchmark"
require "./old_array"

size = 16

old_a1 = OldArray.new(size) { Random::DEFAULT.hex(32) }
old_a2 = OldArray.new(size) { |i| old_a1[i] }

new_a1 = Array.new(size) { |i| old_a1[i] }
new_a2 = Array.new(size) { |i| old_a1[i] }

Benchmark.ips do |x|
  x.report("old") { old_a1 & old_a2 }
  x.report("new") { new_a1 & new_a2 }
end

Result:

old 784.72k (  1.27µs) (± 5.35%)  0.99kB/op   1.56× slower
new   1.23M (814.30ns) (± 5.45%)    448B/op        fastest

uniq

Code:

require "benchmark"
require "./old_array"

size = 16

old_a = OldArray.new(size) { Random::DEFAULT.hex }
new_a = Array.new(size) { |i| old_a[i] }

Benchmark.ips do |x|
  x.report("old") { old_a.uniq }
  x.report("new") { new_a.uniq }
end

Result:

old   1.09M (913.60ns) (± 0.97%)  0.99kB/op   1.96× slower
new   2.14M (466.89ns) (± 2.55%)    448B/op        fastest

uniq!

Code:

require "benchmark"
require "./old_array"

values = Array.new(16) { Random::DEFAULT.hex(64) }

old_a1 = OldArray(typeof(values[0])).new
new_a1 = Array(typeof(values[0])).new

2.times do
  values.each do |value|
    old_a1 << value
    new_a1 << value
  end
end

Benchmark.ips do |x|
  x.report("old") { old_a1.uniq! }
  x.report("new") { new_a1.uniq! }
end

Result:

old 893.02k (  1.12µs) (± 8.08%)  832B/op   3.99× slower
new   3.56M (280.82ns) (± 2.64%)  0.0B/op        fastest
src/array.cr Outdated Show resolved Hide resolved
src/array.cr Show resolved Hide resolved
src/array.cr Outdated Show resolved Hide resolved
@jhass
jhass approved these changes Aug 7, 2019
@asterite asterite added this to the 0.31.0 milestone Aug 7, 2019
@asterite asterite force-pushed the asterite:array-optimizations branch from c111726 to f049db3 Aug 7, 2019
@asterite asterite merged commit 3e49ddf into crystal-lang:master Aug 7, 2019
0 of 5 checks passed
0 of 5 checks passed
ci/circleci: check_format CircleCI is running your tests
Details
ci/circleci: test_darwin CircleCI is running your tests
Details
ci/circleci: test_linux CircleCI is running your tests
Details
ci/circleci: test_linux32 CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@asterite asterite deleted the asterite:array-optimizations branch Aug 7, 2019
@Sija

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

@asterite Is it my code or did this PR introduce a regression?

See:

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

@Sija I don't know, maybe. Do you have a case where it is broken?

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

I mean, show me an array method that works in a wrong way and we can fix it. Otherwise I'm a bit lost regarding what to fix.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

@Sija try doing this:

Money.new(1_00, "EUR") == Money.new(1_00, "USD")

It fails with the same error. Array#& changed a bit and in the optimization it might use == or a hash lookup. It seems the problem is on your side.

@Sija

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

It fails with the same error. Array#& changed a bit and in the optimization it might use == or a hash lookup. It seems the problem is on your side.

That's the thing, before it was using a hash lookup, whereas now it uses<=> through Array#includes? call.

@Sija try doing this:

Money.new(1_00, "EUR") == Money.new(1_00, "USD")

I know it doesn't work, but that's how it's supposed to be. Hash lookup on the other hand is suppose to work based on different semantics (comparing the attributes).

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

No, hash lookup uses first the hash method and then the == method. It's just that before this change a hash was built from the second array and entries were removed based on the first array so it never needed to compare two different currencies. But swap the order of EUR and USD in that spec and I'm sure it will fail before and after this PR.

In every object you must guarantee that if two objects are equal by == then their hash code must be the same (but the reverse is not true).

@Sija

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

But swap the order of EUR and USD in that spec and I'm sure it will fail before and after this PR.

For Crystal 0.30.1 it works in every combination, just tried.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

Well, it's not our problem :-)

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

And with that I mean, there's no problem on our side. I can't understand why you are relying on hash for that to work. As I said, if two objects have the same hash it doesn't mean they are equal. If for some reason the hash seed (which changes from program to program) makes it so that "USD".hash == "EUR".hash then that spec will start failing (depending on the order of the elements in the array).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.