Thanks very much for this. I'll go through it today. @nirvdrum and @eregon can you also take a look? I would say straight away that you still have one failing language spec that needs to be looked at - it doesn't look too complicated though.

We're working on it! Thanks for taking a look.

This is great news. One general comment though, please replace all tab characters with 4 spaces. We should try to keep the formatting consistent with the rest of the code.

Thanks for the feedback, @nirvdrum. We'll fix the indent asap.
@matthias-springer and I will also work on the failing language spec tonight. We already know how to fix it.

Looks good, I had a quick look through the code.
What was the decision behind using a MarkerNode, compared to other alternatives?

We use MarkerNode to indicate that a call site is optimized, i.e., the Hash is expanded and keyword arguments can be read directly from the arguments array instead of the last argument which is a Hash. Now that I though about it again, I think it may be better to add another (boolean) flag to the beginning of the arguments array.

We removed the @Children annotation because we moved the argument nodes to the dispatch node. We should remove the filed from RubyCallNode entirely.

I see, having the (second) marker at the end seems natural as it would be where the Hash is in the non-optimized case. I don't know what is best in case of rest arguments.

@chrisseaton All tests are passing now. Feel free to give more feedback

Still a few tabs, but I'll merge it now so we can get on and run our CI benchmarks. Can you go back and fix the tabs and submit a new PR for that?

@chrisseaton sure, will do. Thanks!

I'm afraid I had to revert these merges temporarily. We've got some regressions on our benchmarks after pulling your branch in, and we don't want to leave any regressions in if we can't fix them in a day as it may mask other regressions.

I've made a branch truffle-kwargs with your changes on them, and we'll try to fix the regressions there. Hopefully it's something minor as it looked like your work shouldn't impact if keyword arguments aren't being used. I'd take an initial look and will let you know what the problem might be, but this might not be until Tuesday.

Hey @chrisseaton,
Do you have any news on this?

I'm going to wait a few days, as we are changing how the PE works in Truffle and probably doing a new release of Graal. It makes sense to wait for those before exploring the problem here.

I don't anticipate it taking long to fix. Do you need it to be merged into master as part of the requirements of your project?

No, I don't think so. The code is already in the repository which should be enough I guess.
I didn't know about the new release of Graal, so I thought it might be better to fix it asap in order to keep the effort as low as possible.

Yeah we won't leave it more than a week max.

No worries. Let us know if we can be of any help. Otherwise, we will leave this with you if that's ok :)

Did you have any benchmarks for testing that keyword arguments were properly optimised?

Yes, sure we did, but they were very simple.

Here's one:

def m(foo:, bar:)
  foo + bar
end

200.times.each do |times|
    beginning_time = Time.now
    (1..1000000).each do |i|
        m(foo: 1, bar: 2)
    end
    end_time = Time.now
    puts "Time elapsed #{(end_time - beginning_time)*1000} milliseconds"
end

and here's a more complex one:

def m(foo:, bar:)
  foo + bar
end

def n(foo = 1, bar = 2)
  foo + bar
end

def o(foo, bar:)
    foo + bar
end

def p(a, b=2, c:)
    a + b + c
end

def q(foo:1, bar:2)
    foo + bar
end

def r(a, b=2, c=3, d:, e:5, f:)
    a+b+c+d+e+f
end

def s(a, b=2, c:, d:, **rest)
    a+b+c+d+rest[:e]
    #rest[:e]
end

def t(**r)
    r[:a]+r[:b]
end

def u(a, b)
    a + b[:a] + b[:b]
end

def assert(a, b)
if a != b
    puts "error: " + a.to_s + "!=" + b.to_s
end
end

200.times.each do |times|
    beginning_time = Time.now
    (1..1000000).each do |i|
        assert(n(1, 2), 3)
        assert(m(foo: 1, bar: 2), 3)
        assert(m({foo: 1, bar:2}), 3)
        assert(o(1, bar: 2), 3)
        assert(o(1, {bar:2}), 3)
        assert(p(1, c:3), 6)
        assert(q(foo:1, bar:2), 3)
        assert(q(foo:1), 3)
        #assert(q(), 3)
        assert(r(1,f:6,d:4), 21)
        assert(s(1,e:5, f:6,d:4,c:3), 15)
        assert(t(a:1, b:2, c:3), 3)
        assert(u(1, {a: 2, b: 3}), 6)
    end
    end_time = Time.now
    puts "Time elapsed #{(end_time - beginning_time)*1000} milliseconds"
end

Maybe I'll just convert some of the existing benchmarks to use kwargs.

Haha, you probably should!