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

[Truffle] Compiled Ruby snippets in Java. #3806

Merged
merged 4 commits into from
Apr 17, 2016
Merged

Conversation

chrisseaton
Copy link
Contributor

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's eval 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 @chumer

Example use: https://github.com/jruby/jruby/compare/truffle-snippets?expand=1#diff-26cfc1760a6d8c34ba92d2e8ea40136dR409

Benchmark:

require 'benchmark/ips'

Benchmark.ips do |x|

  x.iterations = 5

  x.report("concat") do
    "foo".concat(14)
  end

end

MRI:

Warming up --------------------------------------
              concat   156.227k i/100ms
              concat   156.679k i/100ms
              concat   155.219k i/100ms
              concat   137.855k i/100ms
              concat   130.787k i/100ms
Calculating -------------------------------------
              concat      6.173M (± 9.2%) i/s -     30.735M
              concat      6.471M (± 9.7%) i/s -     32.043M
              concat      6.108M (±12.6%) i/s -     29.950M
              concat      5.226M (±26.4%) i/s -     23.411M
              concat      4.956M (±25.6%) i/s -     22.103M

With old ruby():

Warming up --------------------------------------
              concat   317.000  i/100ms
              concat   217.000  i/100ms
              concat   275.000  i/100ms
              concat   301.000  i/100ms
              concat   261.000  i/100ms
Calculating -------------------------------------
              concat      5.766k (± 44.6%) i/s -     25.578k in   5.022988s
              concat      5.065k (± 10.3%) i/s -     25.056k in   5.017998s
              concat      5.158k (± 6.8%) i/s -     25.839k in   5.039072s
              concat      5.262k (± 6.9%) i/s -     26.361k in   5.038556s
              concat      5.411k (± 6.1%) i/s -     27.144k in   5.041226s

With new snippet node:

Warming up --------------------------------------
              concat     3.297k i/100ms
              concat   159.340k i/100ms
              concat   169.392k i/100ms
              concat   247.458k i/100ms
              concat   182.472k i/100ms
Calculating -------------------------------------
              concat      3.119M (± 19.0%) i/s -     13.503M in   5.017577s
              concat      3.162M (± 8.5%) i/s -     15.693M in   5.001186s
              concat      3.326M (± 8.5%) i/s -     16.605M in   5.040981s
              concat      3.196M (± 6.6%) i/s -     16.058M in   5.046234s
              concat      3.330M (± 7.1%) i/s -     16.605M in   5.013123s

JRuby+indy:

Warming up --------------------------------------
              concat   211.919k i/100ms
              concat   241.921k i/100ms
              concat   233.782k i/100ms
              concat   208.173k i/100ms
              concat   244.494k i/100ms
Calculating -------------------------------------
              concat     12.562M (±13.3%) i/s -     60.879M in   4.987114s
              concat     13.098M (±10.3%) i/s -     64.791M in   5.002235s
              concat     13.082M (±10.2%) i/s -     64.791M in   5.006485s
              concat     12.889M (±10.0%) i/s -     63.568M in   5.003120s
              concat     12.906M (± 7.6%) i/s -     64.302M in   5.012395s

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:

Warming up --------------------------------------
              concat    96.939k i/100ms
              concat   161.354k i/100ms
              concat   161.083k i/100ms
              concat   156.297k i/100ms
              concat   159.044k i/100ms
Calculating -------------------------------------
              concat      2.145M (± 5.4%) i/s -     10.815M
              concat      2.162M (± 5.4%) i/s -     10.815M
              concat      2.169M (± 6.0%) i/s -     10.815M
              concat      2.058M (± 7.6%) i/s -     10.338M
              concat      2.132M (± 6.7%) i/s -     10.656M

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

@chrisseaton chrisseaton added this to the truffle-dev milestone Apr 15, 2016
@eregon
Copy link
Member

eregon commented Apr 15, 2016

👍 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).

@eregon
Copy link
Member

eregon commented Apr 15, 2016

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.
A workaround could be:

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]);
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

@chrisseaton
Copy link
Contributor Author

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 CallNode can now have an empty constructor, getting their context and source section from their parent. This will make it much easier to do nodes as @Cached.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@chrisseaton chrisseaton merged commit 0d58bb7 into master Apr 17, 2016
@chrisseaton chrisseaton deleted the truffle-snippets branch April 17, 2016 19:01
@enebo enebo added this to the Non-Release milestone Dec 7, 2017
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

Successfully merging this pull request may close these issues.

None yet

5 participants