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

Optimized Array#skip #6946

Merged
merged 1 commit into from
Oct 16, 2018
Merged

Optimized Array#skip #6946

merged 1 commit into from
Oct 16, 2018

Conversation

asterite
Copy link
Member

Enumerable#skip doesn't know the size of the resulting array, plus it's much efficient to do a mem-copy here.

A benchmark I did:

require "benchmark"

a = Array.new(10_000) { |i| i }

Benchmark.ips do |x|
  x.report("old skip") do
    a.skip(1)
  end
  x.report("new skip 2") do
    a.skip2(1)
  end
  x.report("new skip 3") do
    a.skip3(1)
  end
end

where skip is the current one, skip2 just called [count..-1] and skip3 is the version in this PR.

Results:

  old skip  15.75k ( 63.48µs) (± 3.89%)  99072 B/op   2.62× slower
  new skip 2 41.28k ( 24.23µs) (± 6.37%)  40032 B/op   1.00× slower
new skip 3  41.31k ( 24.21µs) (± 6.05%)  40032 B/op        fastest

@asterite
Copy link
Member Author

BTW, maybe we should rename skip to drop, it reads better. Also for Iterator... I'm thinking that if we want a bang version for this, skip! reads badly, even skip reads badly already.

@yxhuvud
Copy link
Contributor

yxhuvud commented Oct 16, 2018

"BTW, maybe we should rename skip to drop, it reads better."

Also, skip is not an antonym to the opposite, ie take. drop works better from that view too.

src/array.cr Outdated
@@ -946,6 +946,17 @@ class Array(T)
self
end

# Optimized version of `Enumerable#skip`.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this better repeat the description of Enumerable#skip?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Actually, we should have some thingy to inherit doc from the parent. Let's work on that next (there are already a few methods in Array that say they are an optimized version of something else)

Copy link
Contributor

Choose a reason for hiding this comment

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

For now I think it's better indeed, but it would be nice to have a :previous_doc: and/or :super_doc: to automatically copy the doc from previous def or super def or that same method

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied the doc.

@asterite
Copy link
Member Author

@yxhuvud Yeah, take is something else we need. Right now we have first. The problem is that if we have take and first they will be aliases... maybe it's not a big deal. Maybe having aliases is not that bad after all.

@yxhuvud
Copy link
Contributor

yxhuvud commented Oct 16, 2018

Ah right. We have first instead of take, but still have take_while. That is a bit confusing.

@RX14 RX14 added this to the 0.27.0 milestone Oct 16, 2018
@RX14 RX14 merged commit ed033df into crystal-lang:master Oct 16, 2018
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.

7 participants