-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
fast-ruby benchmark comparing #cover to #range is faster on MRI #3841
Comments
Wow isn't it fascinating though that the methods are almost the same ratio compared to each other, on both implementations, even though JRuby is slower overall. |
How peculiar! I know that MRI's Range class has had a lot of optimization hackery over the past several years, so we probably still have inefficient versions of these methods. I'll have a looksee. |
JRuby baseline without indy (indy has a bug that crashes
MRI baseline
Ok, so a few immediate observations about the benchmark:
This is a rather large component of the overall time for at least the
So about 50% better results in both cases. JRuby's still behind, though.
This is very likely where most of the performance is going for at least
Fixing one will fix both. |
Another observation:
This probably won't change things a great deal for the |
A sampled profile (
At first I was baffled to see I think a big part of why the Of course we'd like to run Ruby code as fast as C, but we're not quite there yet :-) I will see if anything can be done with |
Ok, so here's a baseline for the
So we're about 6s off MRI. I made the following changes:
With those changes, here's updated indy numbers:
I showed a second iteration because it was a fair improvement over the first. With these numbers, we're 2/3 of the way to MRI without rewriting |
Additional discovery: We are still not using invokedynamic to bind Java methods, so they go through more machinery and do not optimize/inline as well as they should. This affects the def <=> (other)
case other
when Numeric
ajd <=> other
when Date
# The method compareTo doesn't compare the sub milliseconds so after compare the two dates
# then we have to compare the sub milliseconds to make sure that both are exactly equal.
@dt.compareTo(other.dt).nonzero? || @sub_millis <=> other.sub_millis
else
begin
l, r = other.coerce(self)
l <=> r
rescue NoMethodError
nil
end
end
end So some portion of the remaining gap with MRI is in that Java call not optimizing. Here also you can see the class org::joda::time::DateTime
java_alias :compareDT, :compareTo, [org.joda.time.ReadableInstant]
end
def <=> (other)
if other.kind_of?(Date)
# The method compareTo doesn't compare the sub milliseconds so after compare the two dates
# then we have to compare the sub milliseconds to make sure that both are exactly equal.
@dt.compareDT(other.dt).nonzero? || @sub_millis <=> other.sub_millis
else
__internal_cmp(other)
end
end
private def __internal_cmp(other)
if other.kind_of? Numeric
ajd <=> other
else
begin
l, r = other.coerce(self)
l <=> r
rescue NoMethodError
nil
end
end
end This avoids case/when (because Here's the full diff: diff --git a/core/src/main/java/org/jruby/RubyFixnum.java b/core/src/main/java/org/jruby/RubyFixnum.java
index 202a047..f71470c 100644
--- a/core/src/main/java/org/jruby/RubyFixnum.java
+++ b/core/src/main/java/org/jruby/RubyFixnum.java
@@ -1224,6 +1224,11 @@ public class RubyFixnum extends RubyInteger implements Constantizable {
return input.getRuntime().newFixnum(input.unmarshalInt());
}
+ @Override
+ public IRubyObject nonzero_p(ThreadContext context) {
+ return value == 0 ? context.nil : this;
+ }
+
/* ================
* Singleton Methods
* ================
diff --git a/lib/ruby/stdlib/date.rb b/lib/ruby/stdlib/date.rb
index 75cc0c6..ad36703 100644
--- a/lib/ruby/stdlib/date.rb
+++ b/lib/ruby/stdlib/date.rb
@@ -1402,14 +1402,22 @@ class Date
# two DateTime instances. When comparing a DateTime instance
# with a Date instance, the time of the latter will be
# considered as falling on midnight UTC.
+ class org::joda::time::DateTime
+ java_alias :compareDT, :compareTo, [org.joda.time.ReadableInstant]
+ end
def <=> (other)
- case other
- when Numeric
- ajd <=> other
- when Date
+ if other.kind_of?(Date)
# The method compareTo doesn't compare the sub milliseconds so after compare the two dates
# then we have to compare the sub milliseconds to make sure that both are exactly equal.
- @dt.compareTo(other.dt).nonzero? || @sub_millis <=> other.sub_millis
+ @dt.compareDT(other.dt).nonzero? || @sub_millis <=> other.sub_millis
+ else
+ __internal_cmp(other)
+ end
+ end
+
+ private def __internal_cmp(other)
+ if other.kind_of? Numeric
+ ajd <=> other
else
begin
l, r = other.coerce(self) |
With a temporary fix in place for #3842, here's updated numbers (including all above patches):
This result for Note that |
Updated numbers for MRI 2.3, JRuby 9.1.3.0, and 9.1.3.0 with invokedynamic with only the date.rb patch in place:
There's more to do here but we've improved. 9.1.3.0 also introduces some call site caching into Java code that should make the Fixnum#nonzero? patch unnecessary. |
More to do here in 9.1.4.0. |
We'll do a perf push along with Ruby 2.4 support in JRuby 9.2. |
We are comfortably faster than MRI in both jit modes, and the ratio for Comparable methods is similar to MRI. Calling this fixed. |
|
I ran all the benchmarks from the fast-ruby project and the benchmark that compares #cover to #range seems to be faster on MRI versus JRuby.
Environment
Expected Behavior
Actual Behavior
The text was updated successfully, but these errors were encountered: