-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
[Truffle] Compiled Ruby snippets in Java. #3806
Conversation
👍 Looks good. I think it's good to have a helper method for this anyway as the ruby code might be more than a single method call (Otherwise it should really be specialized with sth like a CallDispatchNode or a shortcut form of it). |
So for this case, I think we could make it simpler. public Object concat(
VirtualFrame frame,
DynamicObject string,
Object other,
@Cached("createCallNode()") CallNode callInternal) {
return callInternal.execute(frame, string, other);
}
protected CallNode createCallNode() {
return new CallNode("concat_internal");
} This would avoid parsing, which seems overkill for this case. Unfortunate indeed we can't have String literals. static final String CONCAT_INTERNAL = "concat_internal";
@Cached("create(CONCAT_INTERNAL)") CallNode callInternal) { |
parameterFrameSlots = new FrameSlot[parameters.length]; | ||
|
||
for (int n = 0; n < parameters.length; n++) { | ||
parameterFrameSlots[n] = frameDescriptor.findOrAddFrameSlot(parameters[n]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just addFrameSlot
here, since duplicated names would be an error I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
Yes I didn't see that this first use case is actually just a basic call and so could be simpler. In related work, I've made it so that internal nodes like |
04fe628
to
f6a2f67
Compare
We use
ruby("...")
to execute Ruby inline in Java, when that's more convenient than creating a load of nodes. It parses each time like MRI'seval
and so obviously doesn't compile and is very slow.This new node allows us to use snippets on the relatively fast path. It compiles the expression once and then calls. It supports inlining etc.
I think it's fast enough to use without worrying too much. I can't really tell though as I think the Ruby code we call using in the first case I've tried it is slow.
I was hoping that using it with
@Cached
would be really elegant, but because the DSL doesn't support string literals I'm afraid that's not quite the case and we need a helper method. I wish we could write something like@CachedStrings("string.concat_internal(other)", "string", "other")
(calling the constructor of the parameter type with those arguments). Even if we could have string literals the escaping would be ugly. cc @chumerExample use: https://github.com/jruby/jruby/compare/truffle-snippets?expand=1#diff-26cfc1760a6d8c34ba92d2e8ea40136dR409
Benchmark:
MRI:
With old
ruby()
:With new snippet node:
JRuby+indy:
So it's 1000x faster than before, but we are still slow and JRuby is 4x faster than us. We're using Rubinius code for this action, and they're very slow, so maybe that's it?
Rubinius:
There are 77 uses of
ruby()
so it will take a little while to replace them. I'm hoping spec times drop when I do.cc @jruby/truffle