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.

Sign up for GitHub

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

Jump to bottom

Fix missing callMethod override #4642

Merged
merged 1 commit into from
Jun 13, 2017
Merged

Fix missing callMethod override #4642

merged 1 commit into from
Jun 13, 2017
+4 −0

Conversation

ivoanjo
Copy link
Contributor

@ivoanjo 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

Fix missing callMethod override
ae3f01e 
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
Copy link
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
Copy link
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
Copy link
Contributor Author

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.

@ivoanjo ivoanjo mentioned this pull request Jun 2, 2017
Open
@headius
Copy link
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
@headius headius added this to the JRuby 9.1.11.0 milestone Jun 13, 2017
@ivoanjo ivoanjo deleted the fix-missing-callmethod-override branch June 13, 2017 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
JRuby 9.1.11.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ivoanjo @kares @headius