-
Notifications
You must be signed in to change notification settings - Fork 0
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 natives #1
base: trunk
Are you sure you want to change the base?
Conversation
Holy shit. This is really cool. Going to see if I can find time to look at this tonight. |
rb_ary_each_with_index(int argc, VALUE *argv, VALUE array) | ||
{ | ||
long i; | ||
volatile VALUE ary = array; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be volatile
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently because clang
: be953b4
It's an odd thing to do, and I don't know if there's any science behind it or if it was just some ✨magic✨ at that point in time.
Only a couple of small comments, but overall this looks really great! |
Would it make sense to include a benchmark/test using an array with mixed types? [1, 'abc', {"key" => "value"}, :symbol] I have no idea if that's going to matter -- just wonder if it'll root out some weird corner cases. |
It won't matter. Most of the array methods don't care about the content of the array, except for some that check for truthiness. This PR passes the MRI test suite as well as That reminds me that I should run the included benchmarks, though, in addition to my micro-benchmarks. |
Can you post the results when you do run those? |
It's been so long since I had a look at any serious C that I don't think my review counts for much, but I didn't see anything objectionable. Awesome work. Hope MRI takes it. 👍 |
{ | ||
ID id; | ||
VALUE op, init = Qnil; | ||
volatile VALUE ary = array; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this local copy of the receiver turned out to be important. Thanks to @grollest for suggesting this fix. There's still a bug lurking in here, but it shows up in the tests rather than some weird failure to build an extension:
#191 test_flow.rb:19:in `<top (required)>':
["a"].inject("ng"){|x,y|
break :ok
}
#=> "ng" (expected "ok")
#193 test_flow.rb:37:in `<top (required)>':
["a"].inject("ng"){|x,y|; $a << 2
break :ok; $a << 3
}; $a << 4
; $a << 5
; rescue Exception; $a << 99; end; $a
#=> "[1, 4, 5]" (expected "[1, 2, 4, 5]")
test_flow.rb FAIL 2/61
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fixed now.
I've addressed review comments & fixed the bugs in |
@fbogsany iamma take a look but we should talk to @csfrancis so we can build it and try IRL |
👍 on trying IRL. |
This adds
Array
-specific implementations of someEnumerable
methods. In some cases (see notes below), two methods are synonymous, butArray
only overrode one of them. I focussed onEnumerable
methods that we actually use in Shopify. There are some more esoteric methods likechunk
,minmax
,slice_before
andslice_after
that I ignored.I've implemented this against a recent MRI
trunk
. It should be a trivial backport to https://github.com/Shopify/ruby.r: @camilo @csfrancis @jasonhl
cc: @grollest @sirupsen
Without change:
With change:
Notes:
include?
andmember?
are synonyms, butmember?
was calling theEnumerable
version of the method. TheArray
version is about 50% faster.flat_map
andcollect_concat
are synonyms. There doesn't seem to be much benefit to a customArray
implementation.to_a
andentries
are synonyms, butentries
was calling theEnumerable
version of the method. TheArray
version is a little faster.select
andfind_all
are synonyms, butfind_all
was calling theEnumerable
version of the method. TheArray
version is about 30% faster.inject
/reduce
is buggy, so I've disabled it for now. Given the complexity of the code, I'm not convinced there's a huge win here, other than removing amemo
object allocation.memo
object allocation - I'll add a few benchmarks with empty arrays to measure the benefit of removing those allocations.grep
benchmark is a bit shady because the block is only called for matching elements, and the content ofarray
is random.Benchmarks: