Override of is_a? causes degraded performance

[thallgren]

The method RGen::MetamodelBuilder::BuilderRuntime#is_a? is currently where the CPU spends most of its time in some of our performance tests. I understand the need for overriding this very basic method to see the ClassModule type inheritance but perhaps there is less intrusive approach than to actually change the is_a? contract?

An alternative that I've experimented with is to implement a specific eIsA? method and then call that method instead when the ClassModule inheritance is relevant. In my experiment I changed the BuilderExtensions.type_check_code slightly so that it tests for eIsA? when the value is an MMBase but use is_a? when it's not. All tests are still green after changing 'test_inheritance' unit test to also use eIsA?. Our own performance tests show a significant improvement.

There are probably more places that needs changing in the Rgen code base (and more tests to write) and I'd be happy to submit a PR for this but I'd like to discuss it first.

[mthiede]

So I understand that the performance bottleneck which you are experiencing is in the type check code and that you have improved that. However I don't understand how you did this. Renaming is_a? to eIsa? and then using the latter instead doesn't seem to change anything. Calling the "normal" is_a? for non-MMBase values also shouldn't make a difference because the special is_a? is only defined for MMBase. Could you pass me the code change you are talking about? e.g. as a pull request?

[thallgren]

Consider cases like:

x.is_a?(String)
x.is_a?(Array)

and x is an MMBase object. It gets really slow in comparison and a special eIsA? removes the overhead simply because it would not be used here. For us, that's not all however. We can safely rely on the standard is_a? at all times. AFAICT, the only occasion where this isn't the case is when there's multiple inheritance involved and we don't use that in our model.

I'm not happy with the eIsA? fix though. I think that solving this problem requires a more extensive change throughout rgen so that inheritance is reflected accurately and the need for a specialized is_a? is removed.

I have some tests (completely separate from rgen) where I instead let all visible model classes be represented by a ruby module and all implementations be a ruby class with _impl suffix that includes the modules that it implements. I also have a factory with create methods for all non-abstract classes. This works well and is very similar to the approach taken in Java. The drawback is that all such a change would break all current rgen deployments.

[mthiede]

I agree that there is a considerable perfromance impact due to the current is_a? solution and that it would be great to improve on that. On the other hand I don't think that using is_a? in the case of metamodel classes is the wrong approach. What sense would it make to let is_a? return true for one super but false for another one? The only solution would be to let it be false for all metamodel superclasses and only return true for MMBase. But then why would I use is_a? at all? Well I could distinguish between a model element and a primitive type value. But when we deal with models we really want to work with metamodel classes as first class citizens. So I really feel the current approach is the most natural one.

Independant of that, even if I wanted to change that, I couldn't do it right now because we have a very large codebase making use of is_a? as it is today. And as you can easily guess, we are using multiple inheritance.

So how about making the definition of the is_a? method in BuilderRuntime optional? Even more simple, you could just monkey-patch it away for your code. Just undefind the method and you should be fine as long as you don't use multiple inheritance: is_a? is fast again and everything alse should work as before.