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

[Truffle] Moving Array#zip to array.rb. #2745

Closed
wants to merge 1 commit into from

Conversation

bjfish
Copy link
Contributor

@bjfish bjfish commented Mar 23, 2015

Could zip be done in ruby? How about insert too?

@chrisseaton
Copy link
Contributor

zip is used in the inner loop of the topaz-neural-net benchmark https://github.com/jruby/bench9000/blob/master/lib/bench/benchmarks/topaz/neural-net.rb#L151, so needs to be fast.

You can see if this commit damages the performance of that benchmark by running jt bench reference topaz-neural-net with the old code, make your changes and then run jt bench compare topaz-neural-net. The margin for error is around ±10%, so maybe run it a few times if it's close. I'm going to guess it isn't close and the current code is much faster.

The same thing applies for insert - it's used by classic-fannkuch-redux so test in the same way.

We can't accept any code that damages the performance of these benchmarks, unless it's fixing an error in our implementation of Ruby. The options are to complete the implementation in Java, or find some way to run Java for the cases we need to be fast, and fallback to Ruby for everything else.

@bjfish
Copy link
Contributor Author

bjfish commented Mar 23, 2015

@chrisseaton I'll check out the benchmarks, I got jt benchto run now, my problem was having GRAAL_BIN set to the graal bin directory and not the Graal java binary.

@bjfish
Copy link
Contributor Author

bjfish commented Mar 23, 2015

@chrisseaton
Any idea how to resolve this warning below?
Also, should I be comparing the 4902.008372064519 or 85.107286 or both? and is smaller or bigger better for the first value?

warning: jruby-9000-dev-truffle-graal topaz-neural-net never warmed up!
topaz-neural-net 4902.008372064519
took 85.107286s

@chrisseaton
Copy link
Contributor

The warning is ok to ignore here. It's just telling you that it has erred on the side of caution because it didn't see the benchmark performance stabilise as much as it would have hoped - that's also why it's run for so long - 85s.

@chrisseaton
Copy link
Contributor

You don't need to look at either of those numbers. reference stores the score in a file. When you run compare it will give you a simple % performance difference.

@bjfish
Copy link
Contributor Author

bjfish commented Mar 23, 2015

It was running 9.36% in the compare.

⚠️ This isn't safe to merge because of a Graal error I need to fix in AtNode.

@bjfish
Copy link
Contributor Author

bjfish commented Mar 23, 2015

Closing due to performance regression

@bjfish bjfish closed this Mar 23, 2015
@chrisseaton chrisseaton added this to the Won't Fix milestone May 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants