[Truffle] Use Array#{max,min} from rubinius #3322
Conversation
|
I have used |
|
I'm waiting on some other changes to be checked carefully for performance regression before I try applying this. Running all of our benchmarks takes a while. |
|
@chrisseaton i'll try this on truffle-head as soon as possible, maybe the errors are different there. |
|
|
|
@eregon yeah, pretty poor implementation, it calls block_given? for each element. but yet we need a proof that it is slower def max
max = undefined
each do
o = Rubinius.single_block_arg
if undefined.equal? max
max = o
else
comp = block_given? ? yield(o, max) : o <=> max
unless comp
raise ArgumentError, "comparison of #{o.class} with #{max} failed"
end
if Comparable.compare_int(comp) > 0
max = o
end
end
end
undefined.equal?(max) ? nil : max
end |
|
@brauliobo the real problem is that |
|
(off topic to this PR) @eregon is it forever game over for methods containing |
|
We have an optimisation in mind to fix The optimisation is simple but will require a bit of refactoring. We should get around to it. |
|
|
|
@chrisseaton will do some benchmarks asap |
|
@brauliobo do you want us to take another look at this PR? If you update it to latest JRuby we'll help you run the benchmarks and see if it needs any more work before it can be merged. |
|
yes @chrisseaton please, really would like to look into this into this but time is running away. thank you :) |
|
Sorry, not sure what you mean. Do you mean you don't have time at the moment, but will do soon? Or you're unlikely to have time to do this any time soon so we should do it? |
|
it is better that you do it because being realistic I won't have enough time for this. |
|
|
Looks great! I'm just setting up a new internal CI and benchmarking system, which I'd like to run this on to give us some more confidence, so I'm sorry I'll just need to delay you by a week or so before we can merge this. |
|
I'm also going to set up some things called PE tests - I'll get back to you about those. |
|
benchmark used: $: << '/home/braulio/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/benchmark-ips-2.6.1/lib'
require 'benchmark/ips'
a = [5,4,10,12,23,3,4]
Benchmark.ips do |x|
x.config time: 5, warmup: 30
x.report{ |t| t.times{ a.max } }
end |
brauliobo
force-pushed
the
test-array-max-min
branch
2 times, most recently
from
8c2ef65
to
6c61494
Compare
|
@chrisseaton updated the benchmarks after doing two separate builds for each branch using |
|
We should revisit now our benchmarks are stable again after some changes to Graal. |
Java's comparable works quite different from the <=> ruby operator
brauliobo
force-pushed
the
test-array-max-min
branch
from
6c61494
to
0f76fcf
Compare
|
master version: ruby version: Performance is roughly the same with constants. |
|
What's going on with those numbers? |
|
Also, I think ironically MRI now has separate methods for |
|
Using a slightly different script of a randomly generated array: $: << '/home/braulio/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/benchmark-ips-2.6.1/lib'
require 'benchmark/ips'
a = 10.times.map{ rand(1..20) }
Benchmark.ips do |x|
x.config time: 5, warmup: 30
x.report{ |t| t.times{ a.max } }
endMakes the java version perform a bit faster: |
|
@chrisseaton good question about the numbers, do you recommend something besides |
|
upgraded to benchmark-ips 2.7.2 and still get the same weird numbers |
|
|
|
I'll pull locally and try tomorrow, and I'll run against out internal CI system as well. I'll update tomorrow. |
|
It looks like this applied cleanly this time and didn't have any impact on performance. Sorry it took a year to get that in! We had the issue with |
|
The final commit applying it is d7ca7b7 |
|
nice @chrisseaton, no worries and thanks! |
Java's comparable works quite differently from the <=> ruby operator
Benchmark:
Rubinius version:
Original version: