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

Fix missing callMethod override #4642

Merged
merged 1 commit into from Jun 13, 2017

Conversation

Projects
None yet
3 participants
@ivoanjo
Contributor

ivoanjo commented Jun 1, 2017

Although not used by default, the Reificator class at https://github.com/jruby/jruby/blob/9.1.10.0/core/src/main/java/org/jruby/RubyClass.java#L1583 checks which version of callMethod it should use, and needs all three versions: no argument, one argument, and more than one argument.

One of these overrides did not exist, and thus would result on a

java.lang.NoSuchMethodError: rubyobj.BrokenReify.callMethod(Ljava/lang/String;Lorg/jruby/runtime/builtin/IRubyObject;)Lorg/jruby/runtime/builtin/IRubyObject;

when running an example with -Xreify.classes=true

class BrokenReify
  def initialize_copy(other)
    super
  end
end

puts BrokenReify.new.clone

With this fix, the above example starts instead giving a java.lang.StackOverflowError due to another bug/bad interaction of the reify.classes option, which I'll report separately.

Issue #4444

Ivo Anjo
Fix missing callMethod override
Although not used by default, the Reificator class at
https://github.com/jruby/jruby/blob/9.1.10.0/core/src/main/java/org/jruby/RubyClass.java#L1583
checks which version of callMethod it should use, and needs all three
versions: no argument, one argument, and more than one argument.

One of these overrides did not exist, and thus would result on a

```
java.lang.NoSuchMethodError: rubyobj.BrokenReify.callMethod(
Ljava/lang/String;Lorg/jruby/runtime/builtin/IRubyObject;)
Lorg/jruby/runtime/builtin/IRubyObject;
```

when running an example with -Xreify.classes=true

```ruby
class BrokenReify
  def initialize_copy(other)
    super
  end
end

puts BrokenReify.new.clone
```

With this fix, the above example starts instead giving a
java.lang.StackOverflowError due to another bug/bad interaction of the
reify.classes option, which I'll report separately.

Issue #4444
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jun 1, 2017

Member

👍

was lately thinking about what needs to be done for -Xreify to be reliable in 9K like it used to be in 1.7.
maybe a fast suite for starters running -Xreify.classes although it might turn out to be a bit much test time.
but I still find reify quite useful with profiling, thread dumping ... any hints you might have @headius ?

Member

kares commented Jun 1, 2017

👍

was lately thinking about what needs to be done for -Xreify to be reliable in 9K like it used to be in 1.7.
maybe a fast suite for starters running -Xreify.classes although it might turn out to be a bit much test time.
but I still find reify quite useful with profiling, thread dumping ... any hints you might have @headius ?

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jun 1, 2017

Member

also this could than incorporate a test such as the one mentioned in the desc.
seems that the RubyClass calls might get eventually deprecated and we should dispatch to what JIT uses.

Member

kares commented Jun 1, 2017

also this could than incorporate a test such as the one mentioned in the desc.
seems that the RubyClass calls might get eventually deprecated and we should dispatch to what JIT uses.

@ivoanjo

This comment has been minimized.

Show comment
Hide comment
@ivoanjo

ivoanjo Jun 1, 2017

Contributor

I plan to open a separate issue, but when I was looking at the code today to debug this I ended up discovering that reify.classes can mostly be used for two purposes:

  1. To have ruby instances that correspond to their class, rather than RubyObject and friends. This is invaluable when debugging memory issues as otherwise it's very hard to see what's going on in VisualVM and friends

  2. To have a Java class that contains the methods of the ruby class, allowing them to be called by Java code

I believe that just having 1) would be extremely valuable. Double so if it was enabled by default.

On the other hand, 2) seems to be what mostly limits this feature, and actually is what makes the test above enter an infinite recursion: RubyKernel calls initialize_copy on the object thinking that RubyBasicObject.initialize_copy will be executed, when in reality it was overriden in the reified class with a trampoline that calls super, and then off we go again.

Contributor

ivoanjo commented Jun 1, 2017

I plan to open a separate issue, but when I was looking at the code today to debug this I ended up discovering that reify.classes can mostly be used for two purposes:

  1. To have ruby instances that correspond to their class, rather than RubyObject and friends. This is invaluable when debugging memory issues as otherwise it's very hard to see what's going on in VisualVM and friends

  2. To have a Java class that contains the methods of the ruby class, allowing them to be called by Java code

I believe that just having 1) would be extremely valuable. Double so if it was enabled by default.

On the other hand, 2) seems to be what mostly limits this feature, and actually is what makes the test above enter an infinite recursion: RubyKernel calls initialize_copy on the object thinking that RubyBasicObject.initialize_copy will be executed, when in reality it was overriden in the reified class with a trampoline that calls super, and then off we go again.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jun 13, 2017

Member

This change is good and I'll merge it. We'll continue discussions about fixing/improving reification in other bugs like #4643.

Member

headius commented Jun 13, 2017

This change is good and I'll merge it. We'll continue discussions about fixing/improving reification in other bugs like #4643.

@headius headius merged commit 20d3053 into jruby:master Jun 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@headius headius added this to the JRuby 9.1.11.0 milestone Jun 13, 2017

@ivoanjo ivoanjo deleted the Talkdesk:fix-missing-callmethod-override branch Jun 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment