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

RubyBasicObject is Redundant (in light of RubyObject) and Breaks Inlining #4763

Open
original-brownbear opened this issue Aug 29, 2017 · 8 comments

Comments

@original-brownbear
Copy link
Contributor

It looks like RubyBasicObject is only extended by RubyObject and doesn't have any other valid standalone use cases (all the static methods on it could just as well live on RubyObject I guess?)

The reason this may be interesting is, that one of the constructors (the one with the ObjectSpace action) cannot be inlined fully at the default inlining depth of 9.

Jitwatch says:

screen shot 2017-08-29 at 16 58 21

Not sure what the quantitative impact is of this, but if I up the inlining depth to 12 it seems the red-black benchmark gets speed up by ~5%.

Maybe there is a quick and easy gain possible here by simply merging RubyBasicObject into RubyObject (or the other way around)?

@enebo
Copy link
Member

enebo commented Aug 29, 2017

I am not against the general idea (rb was bolted onto our impl during 1.9 timeframe) but I think there may be at least one external Java library providing blankslate-like behavior constructing raw RubyBasicObjects. So if we do this then we need to make a new API for making BasicObject instances (which should be super simple) also a nice way to check if something is a BasicObject (from Java) and then deprecate this.

Java integration also internally uses these for java packages so looking at how it is used there might be useful (I think it probably is the two things I mentioned above).

Having to deprecate when it involves a constructor I think would be hard to work around as far as getting the quick gain you see in JITWatch. Possibly we could try class RubyBasicObject implements IRubyObject to maintain backwards compat? Seems likely to run aground but maybe that would be a path forward without waiting for major release.

The other question to ask is this analogue worth having? RubyBasicObject -> BasicObject

@kares
Copy link
Member

kares commented Aug 31, 2017

Java integration also internally uses these for java packages so looking at how it is used there might be useful (I think it probably is the two things I mentioned above).

currently not - seems that we're still able to HACK around using the blank-state-wrapper (for Java packages - so that methods are not triggered but resolve as package names) while wrapping a package instance in a module instead of BasicObject (there's report for not having a pkg.superclass but it somehow got working without switching to BasicObject - so far).

@headius
Copy link
Member

headius commented Sep 5, 2017

This is definitely a tricky problem to work around. The 9-deep inlining is specific to Hotspot, so that may not have the same effect under e.g. Graal or J9. But Hotspot is obviously going to be the widest-deployed JVM for a long time.

This is particularly problematic for some of the heaviest-used objects in JRuby, like numbers.

For example, the path to construct a Fixnum object needs to go through at least six constructors (RubyFixnum < RubyInteger < RubyNumeric < RubyObject < RubyBasicObject < Object) and typically will go through at least a couple overloads, a factory method, and usually a Ruby method like "+" or "-" that has multiple layers. It's quite easy then to exceed 9 frames.

Because of the way Java handles constructors, the only way to remove any of these levels is to remove the class (you can't skip your immediate parent class's constructor). So we're back to your suggestion of removing -- in some way -- RubyBasicObject. That would be an API challenge since this type has existed for many years and there are likely many libraries that reference it as a class (so we can't simply make it an interface).

The other option we have not done recently is to audit all constructor paths for the key core Ruby types to ensure they're as short as possible; this may mean duplicating constructor or factory methods to avoid extra levels of dispatch. I'll take a quick look at that angle.

@headius
Copy link
Member

headius commented Sep 5, 2017

Bleh, the removal or skipping of factory methods is also problematic, because the factory methods check the width of the fixnum cache. There's no way to have a constructor call return a cached object, and no way to have a factory method skip the constructor if it needs to make a new object.

So any of these factory methods we want to eliminate would have to move the cache hit up to the caller.

@headius
Copy link
Member

headius commented Sep 5, 2017

Ok, for a simple numeric benchmark, reducing the number of frames by one or two doesn't appear to help Fixnum creation a great deal. It may help more in cases where there's no conditional logic wrapping the construction (e.g. temporary Array wrappers for args), since those cases will fare better in Hotspot's escape analysis.

@headius
Copy link
Member

headius commented Sep 5, 2017

There's another possible option: reverse the relationship of RubyObject and RubyBasicObject.

All classes in Ruby declared through normal means will extend Object or some subclass of it, meaning 99% of objects in the system will be or descend from RubyObject.

The remaining small percentage of objects will be RubyBasicObject. All cases where the superclass is BasicObject will be RubyBasicObject instances. All other cases will be RubyObject. To me this makes the two base types somewhat disjoint.

There are instanceof checks that could be problematic; e.g. RubyBasicObject is currently an Object but not a RubyObject. With this change, that would reverse; RubyObject would not longer be a RubyBasicObject, but RubyBasicObject would be a RubyObject.

Another idea: make RubyBasicObject function as a standalone type, but don't use it normally for objects in Ruby based on BasicObject. That would allow Java APIs to continue to work with RubyBasicObject, but internally we would treat that and RubyObject+BasicObject metaclass as the same thing.

@headius
Copy link
Member

headius commented Sep 5, 2017

There's another value in providing special roots for the hierarchy: immediate types could be modified to use alternative instance variable stores (or none at all) saving 32-64 bits of header space on those objects. This could also be applied to flags, if we had a base class that always made a frozen, immediate object and a base class that had configurable flags.

@headius
Copy link
Member

headius commented Sep 5, 2017

Ok, I did some experimentation along these lines.

I think this is only really feasible to tackle along with a move to Java 8, since we get default interface methods. Default methods would help us pull interfaces out for some of these core types that don't really need standalone classes, like RubyNumeric and RubyInteger. RubyNumeric, for example, could become an "RNumeric" interface with all the standard impls as default methods. Then anything that currently extends RubyNumeric could simply extend RubyObject and implement RNumeric.

This would make it possible to arbitrarily flatten almost any hierarchy. Targeted use for immediate numbers could reduce the size of floats and fixnums by 2-4 fields, potentially saving many bytes per object.

Of course there's a lot of challenges to making this move:

  • API compatibility on those disappearing types would be hard to maintain. Even if we transparently made the interface look like the class, all dispatches to those methods would have to be recompiled as invokeinterface.
  • Type checks would have to be smart about the interface types and the flattened hierarchy. RubyFixnum would no longer be instanceof RubyNumeric or RubyInteger, but would be instanceof RNumeric and RInteger.
  • There may be unknown overhead in making classes with large numbers of default interface impls.
  • There's a lot of methods to transplant in this way.

I think Java 8 is a prerequisite for this work in any case.

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

No branches or pull requests

4 participants