KindOf implementation using Class.isInstance() instead of instanceof #614

Closed
ghost opened this Issue Mar 29, 2013 · 7 comments

Projects

None yet

2 participants

@ghost
ghost commented Mar 29, 2013

This would probably be an good entry-point for someone looking to get involved in hacking JRuby.

There are a heap of single-method KindOf classes that look like:

fixnum.kindOf = new RubyModule.KindOf() {
    @Override
    public boolean isKindOf(IRubyObject obj, RubyModule type) {
        return obj instanceof RubyFixnum;
    }
}

Most of these could be replaced with a single implementation that uses Class.isInstance()
e.g.

public static final class JavaClassKindOf extends RubyModule.KindOf {
    private final Class klass;
    public JavaClassKindOf(Class klass) { 
        this.klass = klass; 
    }

    @Override
    public boolean isKindOf(IRubyObject obj, RubyModule type) {
        return klass.isInstance(obj);
    }
}

This wrinkle would be to benchmark this to make sure hotspot is optimising it the same way - intrinsic isInstance() might only be in very recent hotspot.

@influenza
Contributor

I've implemented this on my fork - how would I go about getting the benchmark numbers to ensure this change hasn't degraded performance?

@influenza
Contributor

According to the Oracle HotSpot docs, "Class.isInstance and Class.isAssignableFrom are as cheap as instanceof bytecodes when the operands are constants, and otherwise no more expensive than aastore type checks".

@headius
Member
headius commented May 21, 2013

@influenza For benchmarking, A trivial benchmark that just tries instanceof with various combinations of objects (core versus user-defined, simple versus complex hierarchies) would show you if anything has degraded.

Getting rid of these little classes would be great. We will fall into the latter category, with no constant class passed to isInstance, but I suspect it will not hurt performance any.

@influenza
Contributor

I've posted the benchmark code that I used in this gist and this gist. According to the results, the changes I introduced have caused about a 2% speedup when compared against the released JRuby 1.7.3 - when compared against a build from the commit prior to my changes, this is about a 0.7% degradation in performance. To get these results, I ran each of the tests five times. With that being said, I think that the changes on the master branch since the 1.7.3 release was cut has improved the performance far more than this change has degraded it. I of the opinion that the degradation is negligible. We may even see some small amount of savings in the amount of PermGenSpace claimed, as we will have fewer unique anonymous inner classes.

@influenza
Contributor

Pull request submitted

@ghost
ghost commented May 24, 2013

I think that benchmark might be overwhelmed by the array appending.

Try a slightly different approach:

  • fill an array with say 100k elements - split between each of the First ... Seventh types. It might also be an idea to make a separate array of instances of core classes affected by this change.
  • in each iteration of the loop, compare an array element against each of the core class types, and update a boolean var with the result (e.g. using the |= or &= operator). I'd also separately benchmark the non-core ruby classes vs user defined classes. The core classes are the ones which have their own KindOf impl, so are most likely to be affected by this patch.
  • probably also unroll the loop a bit, so it is doing N array elements per iteration, to remove as much loop overhead as possible.
  • there also needs to be a warmup run to make sure hotspot isn't still optimising when the actual benchmark times are being recorded.

As an example, see https://github.com/jruby/jruby/blob/master/bench/ffi/bench_getpid.rb - that uses the Benchmark lib, and unrolls the inner loops.

@influenza
Contributor

Will do! Thanks for the feedback

@headius headius added a commit that closed this issue Jun 10, 2013
@influenza @headius influenza + headius Use RubyModule.JavaClassKindOf fixes jruby/jruby#614
I've changed all of the relevant cases of anonymous KindOf classes to
use the new JavaClassKindOf. This cuts down on some of the noise
produced by anonymous inner classes, especially given that these
replacement points all had identical implementations.
bcc2674
@headius headius closed this in bcc2674 Jun 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment