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

Optimise Indexable#== #4045

Merged
merged 1 commit into from
Feb 20, 2017
Merged

Optimise Indexable#== #4045

merged 1 commit into from
Feb 20, 2017

Conversation

karlseguin
Copy link
Contributor

Use unsafe_at on other as the size has already been checked.

@ysbaddaden
Copy link
Contributor

We shouldn't have i going outbound, so it should be fine.

Any benchmark to show the actual improvement?

@sdogruyol
Copy link
Member

I just did a benchmark for this and it seems a tad bit faster (based on 5 runs)

require "benchmark"

a = [1, 2]
b = [1, 2]

Benchmark.ips do |x|
  x.report("Equals") do
    a.equals?(b) { |x, y| x == y }
  end
end

With 0.25.0

Equals 204.81M (  4.88ns) (±13.07%) fastest
Equals 205.21M (  4.87ns) (±13.13%) fastest
Equals 205.97M (  4.86ns) (±12.68%) fastest
Equals 209.17M (  4.78ns) (±10.84%) fastest
Equals 208.29M (   4.8ns) (±11.28%) fastest

With PR

Equals 206.51M (  4.84ns) (±11.84%) fastest
Equals 206.13M (  4.85ns) (±12.10%) fastest
Equals 206.22M (  4.85ns) (±12.35%) fastest
Equals 208.89M (  4.79ns) (± 9.73%) fastest
Equals 211.72M (  4.72ns) (± 9.56%) fastest

@asterite
Copy link
Member

We can't assume other is an Indexable, just that it has size and [].

@karlseguin
Copy link
Contributor Author

karlseguin commented Feb 17, 2017

@asterite I see what you mean, then this isn't any good as unsafe_at might not be available.

I guess one alternative would be to overload. Would you recommend that?

I'm pretty new here...can you explain why it's safe to assume [] but not unsafe_at? I'm not doubting/question you, but I'm curious if that's documeted/enforced or is it up to stdlib developers to vigilantly never accidentally introduce a restriction?

@karlseguin
Copy link
Contributor Author

FWIW, the performance gains are more significant with a larger array.

a = Array.new(100) {|i| i}
b = Array.new(100) {|i| i}

0.25.0

Equals    8.6M (116.22ns) (± 6.46%) fastest
Equals   8.53M (117.25ns) (± 6.39%) fastest
Equals   8.57M (116.66ns) (± 6.46%) fastest
Equals    8.6M (116.33ns) (± 6.26%) fastest
Equals   8.55M (117.02ns) (± 6.30%) fastest

PR

Equals  16.25M ( 61.54ns) (± 8.99%) fastest
Equals  16.28M ( 61.43ns) (± 9.14%) fastest
Equals  16.34M ( 61.21ns) (± 8.92%) fastest
Equals  16.25M ( 61.56ns) (± 8.77%) fastest
Equals  16.25M ( 61.56ns) (± 8.91%) fastest

@asterite
Copy link
Member

@karlseguin For example if I have this:

class MyIndexable
  getter size

  def initialize(@size : Int32)
  end

  def [](index)
    if 0 <= index < @size
      index
    else
      raise IndexError.new
    end
  end
end

a = [0, 1, 2]
b = MyIndexable.new(3)
p a.equals?(b) { |x, y| x == y }

It's a super hypothetical case, though. What we can do is to overload equals?, one with an Indexable restriction and without it that would use #[]. What do you think?

src/indexable.cr Outdated
@@ -211,6 +211,16 @@ module Indexable(T)
size == 0
end

# Optimized version of `Indexable#equals?` used when `other` is also an
# `Indexable`.
def equals?(other : Indexable(T))
Copy link
Member

Choose a reason for hiding this comment

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

There's no need to restrict against T, just Indexable is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, didn't know. Thanks.

Do you know how to get the docs to link to the correct overload? Couldn't figure it out.

other's value through unsafe_at as its size has already been checked.
@asterite asterite added this to the 0.21.0 milestone Feb 20, 2017
@asterite asterite merged commit 44c859f into crystal-lang:master Feb 20, 2017
@asterite
Copy link
Member

@karlseguin Thank you for this! 💚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants