Project

General

Profile

Misc #16188

What are the performance implications of the new keyword arguments in 2.7 and 3.0?

Added by Eregon (Benoit Daloze) about 2 months ago. Updated 25 days ago.

Status:
Open
Priority:
Normal
[ruby-core:95148]

Description

In #14183, keyword arguments became further separated from positional arguments.

Contrary to the original design though, keyword and positional arguments are not fully separated for methods not accepting keyword arguments.
Example: foo(key: :value) will def foo(hash) will pass a positional argument.
This is of course better for compatibility, but I wonder what are the performance implications.

The block argument is completely separate in all versions, so no need to concern ourselves about that.

In Ruby <= 2.6:

  • The caller never needs to know about the callee's arguments, it can just take all arguments and pass them as an array. The last argument might be used to extract keyword, but this is all done at the callee side.
  • Splitting kwargs composed of Symbol and non-Symbol keys can be fairly expensive, but it is a rare occurrence. If inlining the callee and kwargs are all passed as a literal Hash at the call site, there shouldn't be any overhead compared to positional arguments once JIT'ed.

In Ruby 2.7:

  • The caller needs to pass positional and keyword arguments separately, at least when calling a method accepting kwargs. But, if it calls a methods not accepting kwargs, then the "kwargs" (e.g. foo(key: :value)) should be treated just like a final Hash positional argument.
  • (If we had complete separation, then we could always pass positional and keyword arguments separately, so the caller could once again ignore the callee)

How is the logic implemented in MRI for 2.7?

Specializing the caller for a given callee is a well-known technique.
However, it becomes more difficult if different methods are called from the same callsite (polymorphic call), especially if one accepts kwargs and another does not.
In that case, I think we will see a performance cost to this approach, by having to pass arguments differently based on the method to be called.

What about delegation using ruby2_keywords?
Which checks does that add (compared to 2.6) in the merged approach with the Hash flag?


Related issues

Related to Ruby master - Feature #14183: "Real" keyword argumentClosedActions
Related to Ruby master - Misc #16157: What is the correct and *portable* way to do generic delegation?OpenActions

History

#1

Updated by Eregon (Benoit Daloze) about 2 months ago

#2

Updated by Eregon (Benoit Daloze) about 2 months ago

  • Description updated (diff)

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

Eregon (Benoit Daloze) wrote:

In #14183, keyword arguments became further separated from positional arguments.

Contrary to the original design though, keyword and positional arguments are not fully separated for methods not accepting keyword arguments.
Example: foo(key: :value) will def foo(hash) will pass a positional argument.
This is of course better for compatibility, but I wonder what are the performance implications.

Internally, there are not really performance implications for the choice to treat keyword arguments as a last positional hash. If we not do so, the foo(key: value) call would be an ArgumentError. I don't believe we had to give up any optimizations in CRuby when I took mame's original branch and added backwards compatibility to treat keyword arguments as a last positional hash.

In Ruby <= 2.6:

  • The caller never needs to know about the callee's arguments, it can just take all arguments and pass them as an array. The last argument might be used to extract keyword, but this is all done at the callee side.
  • Splitting kwargs composed of Symbol and non-Symbol keys can be fairly expensive, but it is a rare occurrence. If inlining the callee and kwargs are all passed as a literal Hash at the call site, there shouldn't be any overhead compared to positional arguments once JIT'ed.

If you pass a literal hash to a method that accepts a keyword splat (where the literal hash is treated as keywords), I assume there still must be overhead compared to passing a literal hash to a method that it does not accept keyword arguments, as it must allocate a new hash. I think passing a literal hash to a method that accepts explicit keywords but no keyword hash can be allocation-less. I'm not sure if the JIT engine change things in this area, maybe k0kobun knows.

In Ruby 2.7:

  • The caller needs to pass positional and keyword arguments separately, at least when calling a method accepting kwargs. But, if it calls a methods not accepting kwargs, then the "kwargs" (e.g. foo(key: :value)) should be treated just like a final Hash positional argument.
  • (If we had complete separation, then we could always pass positional and keyword arguments separately, so the caller could once again ignore the callee)

How is the logic implemented in MRI for 2.7?

The general structure remains the same. There are warnings added for methods that accept keyword arguments that will break in Ruby 3:

  • Treating a last hash as keywords.
  • Treat keywords as a final mandatory positional argument.
  • Splitting last hash or keywords when explicit keywords given and keyword splat not accepted.

There are some behavior changes compared to 2.7:

  • No need to split keyword hash (or last hash treated as keywords) if a keyword splast is accepted, as keyword splats accept non-Symbol keys in 2.7.
  • **nil is now supported in method definitions for treating the method like it accepts keywords, but not have any keywords accepted.
  • Passing an empty keyword splat to a method no longer passes an argument, unless this argument is a required positional argument, in which case it is warned.

This last point is the one behavior in CRuby 2.7 that has significant performance implications. What happens is an empty hash argument is not passed, but the call is flagged that empty keyword hash was passed. If the empty keyword hash turns out to be required to fulfill a positional argument, an empty hash is added back. This requires allocating a temporary buffer to store the new argument array.

Specializing the caller for a given callee is a well-known technique.
However, it becomes more difficult if different methods are called from the same callsite (polymorphic call), especially if one accepts kwargs and another does not.
In that case, I think we will see a performance cost to this approach, by having to pass arguments differently based on the method to be called.

I don't think this is true in CRuby. It certainly could be true in other Ruby implementations. However, I would think you could always treat things as passing keywords in the caller code, and just have the callee convert keywords to a positional hash if the method does not accept keywords.

What about delegation using ruby2_keywords?
Which checks does that add (compared to 2.6) in the merged approach with the Hash flag?

For methods that are flagged with ruby2_keywords (which can only happen if the method accepts a regular splat but no keywords or keyword splat), if the method is called with keywords, we set a flag so that the keyword hash is treated as a last positional hash with the keyword flag set. Additionally, in this case the empty keyword hash is not removed, it is treated the same as a non-empty keyword hash.

For all method calls that use an argument splat and pass no keywords or keyword splat, if the last element of the argument splat array is a hash with the keyword flag set, that argument is treated as keywords instead of a positional hash.

#4

Updated by Eregon (Benoit Daloze) about 1 month ago

  • Related to Misc #16157: What is the correct and *portable* way to do generic delegation? added

Updated by Eregon (Benoit Daloze) about 1 month ago

jeremyevans0 (Jeremy Evans) wrote:

Thank you for your detailed reply.

Internally, there are not really performance implications for the choice to treat keyword arguments as a last positional hash. If we not do so, the foo(key: value) call would be an ArgumentError.

I think there are. If args and kwargs are completely separated, we can choose to represent kwargs as we wish, and only need to build a Hash if the method takes a **kwrest.
But in the current approach, for a call site calling both a method taking kwargs and another not taking kwargs, we'll need logic to handle both the kwargs case and the conversion to a last positional Hash. If they were completely separated, the positional Hash would just be an ArgumentError and we wouldn't need that logic at all.
I agree that would be too incompatible though, so let's focus on optimizing the semantics in Ruby 2.7 and for Ruby 3.

I don't believe we had to give up any optimizations in CRuby when I took mame's original branch and added backwards compatibility to treat keyword arguments as a last positional hash.

I think CRuby does not do many optimizations for keyword arguments except for the simplest case, I'd like to consider optimizations and JITs in general.
A few extra checks in MRI might not be noticeable if some form of kwargs (e.g., **kwargs) is generally slow, but could very well be significant on a more optimizing Ruby implementation.

If you pass a literal hash to a method that accepts a keyword splat (where the literal hash is treated as keywords), I assume there still must be overhead compared to passing a literal hash to a method that it does not accept keyword arguments, as it must allocate a new hash.

**kwrest allocates conceptually, but the allocation does not need to happen, for instance if the Hash instance is not stored anywhere and escape analysis can figure it out.
This is how keyword arguments are currently optimized in TruffleRuby: we always create the Hash, but the escape analysis in the JIT figures it does not need to actually allocate it in many cases.

I think passing a literal hash to a method that accepts explicit keywords but no keyword hash can be allocation-less. I'm not sure if the JIT engine change things in this area, maybe k0kobun knows.

When I mentioned JIT, I was thinking specifically to TruffleRuby, JRuby and highly-optimizing JITs.
I think MJIT doesn't optimize kwargs better than the interpreter currently.

The general structure remains the same. There are warnings added for methods that accept keyword arguments that will break in Ruby 3:

Right, what about Ruby 3?
We should aim something that is both semantically simpler and easier to optimize then.

There are some behavior changes compared to 2.7:

  • No need to split keyword hash (or last hash treated as keywords) if a keyword splast is accepted, as keyword splats accept non-Symbol keys in 2.7.

That should be good for performance, no more "crazy" splitting in argument handling and complicated checks for whether splitting should be tried :)

  • Passing an empty keyword splat to a method no longer passes an argument, unless this argument is a required positional argument, in which case it is warned.

This last point is the one behavior in CRuby 2.7 that has significant performance implications. What happens is an empty hash argument is not passed, but the call is flagged that empty keyword hash was passed. If the empty keyword hash turns out to be required to fulfill a positional argument, an empty hash is added back. This requires allocating a temporary buffer to store the new argument array.

Right, so we need to pass an extra flag with the call for that case at least.

How are *args and **kwargs actually represented in MRI?
Is it like a flat array of "arguments", and the last one can be a keyword Hash, and there is some flag passed to the callee to differentiate a last positional Hash from keyword arguments?
Then methods taking kwargs need to check the flag to know how many positional args are passed, and whether kwargs are passed.
Methods not taking kwargs don't even need to check the flag, and can consider the entire flat array as positional arguments.

I guess cases like foo(a: 1, b: 2) are not represented with a Hash to avoid allocations in MRI, how does it look like then for such a case?
That makes the case for methods not taking kwargs quite more complicated, as they need to read the "passed optimized kwargs" flag and if set convert those optimized kwargs to a Hash, and add + 1 to the count of positional arguments.

I don't think this is true in CRuby. It certainly could be true in other Ruby implementations. However, I would think you could always treat things as passing keywords in the caller code, and just have the callee convert keywords to a positional hash if the method does not accept keywords.

Right, that sounds like the easiest approach.

For methods that are flagged with ruby2_keywords (which can only happen if the method accepts a regular splat but no keywords or keyword splat), if the method is called with keywords, we set a flag so that the keyword hash is treated as a last positional hash with the keyword flag set. Additionally, in this case the empty keyword hash is not removed, it is treated the same as a non-empty keyword hash.

That should be fine, because it means only ruby2_keywords flagged methods would have a little overhead for that extra logic (i.e., writing a flag or @ivar on the Hash).
Deciding to keep or remove the empty keyword hash case should itself be free once JITed, as it's a never-changing property of a method, if we make ruby2_keywords define a new method internally.

For all method calls that use an argument splat and pass no keywords or keyword splat, if the last element of the argument splat array is a hash with the keyword flag set, that argument is treated as keywords instead of a positional hash.

That's the one I'm concerned about, it means every foo(some arguments) with def foo(*args); bar(*args); end will have to check if the last positional argument is a Hash with the flag set:
args.size >= 1 && Hash === args[-1] && flagged?(args[-1]). That's a quite a few branches and it's not just reading one bit from memory, it's at least 4 memory loads (size, [-1], .class, .flag).
I think this is another motivation for having ruby2_keywords only in Ruby 2.7, as it's clearly not free for performance.

We could skip that if we know currently no method uses ruby2_keywords, but as soon as it's used we have to perform the check for every method call with the method taking a rest argument (and no explicit kwargs).
I would think there are quite a few methods taking a rest argument and no explicit kwargs, i.e. def foo(*args), def foo(req, *args), def foo(opt=default, *args), etc all need the extra check
but def foo(*args, **kwargs)/def foo(*args, kwarg:)/def foo(*args, kwarg: default) do not need the check.

Maybe we should be more explicit about when to convert a flagged Hash to keyword arguments.
For instance we could have, using ActionDispatch::MiddlewareStack::Middleware's example:

class Middleware
  attr_reader :args, :block, :klass

  def initialize(klass, args, block)
    @klass = klass
    @args  = args
    @block = block
  end
  ruby2_keywords :initialize if respond_to?(:ruby2_keywords, true)

  # Current code
  def build(app)
    klass.new(app, *args, &block)
  end

  # idea to be explicit when converting to keyword args
  # and avoid extra checks in other call to methods with a *rest argument.
  def build(app)
    klass.send_keyword_hash(:new, app, *args, &block)
  end
end

That would limit that complicated check to only send_pass_kwargs calls, and not any other calls.
It's also more explicit and less magic, which might be easier to understand and explain.
I'm unsure about a good name, maybe send_pass_kwargs, send_pass_flagged_kwargs, send_pass_flagged_hash, send_flagged_hash_as_kwargs, send_kwhash, send_keyword_hash?
What do you think?

Updated by jeremyevans0 (Jeremy Evans) about 1 month ago

Eregon (Benoit Daloze) wrote:

jeremyevans0 (Jeremy Evans) wrote:

Internally, there are not really performance implications for the choice to treat keyword arguments as a last positional hash. If we not do so, the foo(key: value) call would be an ArgumentError.

I think there are. If args and kwargs are completely separated, we can choose to represent kwargs as we wish, and only need to build a Hash if the method takes a **kwrest.
But in the current approach, for a call site calling both a method taking kwargs and another not taking kwargs, we'll need logic to handle both the kwargs case and the conversion to a last positional Hash. If they were completely separated, the positional Hash would just be an ArgumentError and we wouldn't need that logic at all.
I agree that would be too incompatible though, so let's focus on optimizing the semantics in Ruby 2.7 and for Ruby 3.

I should preface this by saying all of my comments related to performance implications were and are implicitly refering to CRuby's implementation. I have no knowledge of the internals of other Ruby implementations and no idea what the performance implications of the differences are on other implementations.

Note that when discussing performance implications of keyword arguments, we should be clear what we are discussing. In Ruby 2.7 and 3.0, the caller still doesn't need to know about the callee's argument handling, it just passes all arguments through. It is up to the callee to raise an ArgumentError if the arguments are not correct. This is the same as older Ruby versions. Just as in older Ruby versions, the callee handles treating keywords as a final positional hash if the method does not accept keyword arguments. The only thing that is really changed is how the callee is handling the arguments, the caller code is generally unaffected.

The general structure remains the same. There are warnings added for methods that accept keyword arguments that will break in Ruby 3:

Right, what about Ruby 3?
We should aim something that is both semantically simpler and easier to optimize then.

If you want to see what Ruby 3 keyword argument handling will look like, I have a branch prepared for that already: https://github.com/jeremyevans/ruby/tree/r3

I have been using this for testing my apps and libraries to be sure that any case that will break in Ruby 3 will be warned in Ruby 2.7. Most of the diff for the core changes is code removal, there are very few additions: https://github.com/jeremyevans/ruby/commit/6aa7d021f694537ad15d3f72941e4816639ddfd5

The code does end up being semantically simpler, and it should be faster, though I don't expect the performance to be significantly better in real-world cases.

How are *args and **kwargs actually represented in MRI?
Is it like a flat array of "arguments", and the last one can be a keyword Hash, and there is some flag passed to the callee to differentiate a last positional Hash from keyword arguments?
Then methods taking kwargs need to check the flag to know how many positional args are passed, and whether kwargs are passed.
Methods not taking kwargs don't even need to check the flag, and can consider the entire flat array as positional arguments.

In most of the C-API, int argc, VALUE *argv, with the addition of int kw_splat in Ruby 2.7 for whether the final argument is a hash that should be treated as keywords.

I guess cases like foo(a: 1, b: 2) are not represented with a Hash to avoid allocations in MRI, how does it look like then for such a case?

There is an optimization for literal keyword use in both the caller and callee (if both are written in Ruby) so that a hash is not allocated in that case if no keyword splats are present in the caller and callee.

That makes the case for methods not taking kwargs quite more complicated, as they need to read the "passed optimized kwargs" flag and if set convert those optimized kwargs to a Hash, and add + 1 to the count of positional arguments.

In CRuby, it's not significantly more complicated. The callee may need to add the literal keywords to a hash anyway (if callee uses a keyword splat). Moving that hash to the last positional argument is very little work.

For methods that are flagged with ruby2_keywords (which can only happen if the method accepts a regular splat but no keywords or keyword splat), if the method is called with keywords, we set a flag so that the keyword hash is treated as a last positional hash with the keyword flag set. Additionally, in this case the empty keyword hash is not removed, it is treated the same as a non-empty keyword hash.

That should be fine, because it means only ruby2_keywords flagged methods would have a little overhead for that extra logic (i.e., writing a flag or @ivar on the Hash).

Correct, the check to add the flag is only on ruby2_keywords flagged methods.

For all method calls that use an argument splat and pass no keywords or keyword splat, if the last element of the argument splat array is a hash with the keyword flag set, that argument is treated as keywords instead of a positional hash.

That's the one I'm concerned about, it means every foo(some arguments) with def foo(*args); bar(*args); end will have to check if the last positional argument is a Hash with the flag set:
args.size >= 1 && Hash === args[-1] && flagged?(args[-1]). That's a quite a few branches and it's not just reading one bit from memory, it's at least 4 memory loads (size, [-1], .class, .flag).
I think this is another motivation for having ruby2_keywords only in Ruby 2.7, as it's clearly not free for performance.

It's not free in CRuby. However, if you look at all of the surrounding code, I don't think the performance difference in CRuby will be significant.

I would think there are quite a few methods taking a rest argument and no explicit kwargs, i.e. def foo(*args), def foo(req, *args), def foo(opt=default, *args), etc all need the extra check
We could skip that if we know currently no method uses ruby2_keywords, but as soon as it's used we have to perform the check for every method call with the method taking a rest argument (and no explicit kwargs).

A sufficiently advanced compiler in a more optimized Ruby implementation should be able to determine in most cases whether the method is ever called with such a hash, and optimize the check out, correct? :)

Consider:

ruby2_keywords def foo(*a) bar(*a) end

Other than the check at this specific bar call-site, you don't need to check other call sites. Because when bar is called here, the hash with the flag is duped and the flag removed (as if the hash was keyword splatted), so the flagged hash never escapes this method. This is not how the master branch works in CRuby, but I consider that a bug, and I'll be fixing it shortly.

Handling more involved cases where the arguments are stored and can escape may require checking all call sites with splats and no explicit keywords, though.

but def foo(*args, **kwargs)/def foo(*args, kwarg:)/def foo(*args, kwarg: default) do not need the check.

correct.

Maybe we should be more explicit about when to convert a flagged Hash to keyword arguments.
For instance we could have, using ActionDispatch::MiddlewareStack::Middleware's example:

class Middleware
  attr_reader :args, :block, :klass

  def initialize(klass, args, block)
    @klass = klass
    @args  = args
    @block = block
  end
  ruby2_keywords :initialize if respond_to?(:ruby2_keywords, true)

  # Current code
  def build(app)
    klass.new(app, *args, &block)
  end

  # idea to be explicit when converting to keyword args
  # and avoid extra checks in other call to methods with a *rest argument.
  def build(app)
    klass.send_keyword_hash(:new, app, *args, &block)
  end
end

That would limit that complicated check to only send_pass_kwargs calls, and not any other calls.
It's also more explicit and less magic, which might be easier to understand and explain.
I'm unsure about a good name, maybe send_pass_kwargs, send_pass_flagged_kwargs, send_pass_flagged_hash, send_flagged_hash_as_kwargs, send_kwhash, send_keyword_hash?
What do you think?

send_keyword_hash is more explicit, but it requires significantly more code or a global monkey patch of Object#send_keyword_hash to be backwards compatible. The main goal for ruby2_keywords is to allow existing method definitions to work without changing them, just by flagging the method.

I assume in your example, initialize should accept *args and not args, as otherwise, you can't use ruby2_keywords.

It seems odd to recommend we add another feature (Kernel#send_keyword_hash) to compliment a feature (Module#ruby2_keywords) that you are strongly recommending we remove in Ruby 3.0.

Updated by Eregon (Benoit Daloze) 30 days ago

jeremyevans0 (Jeremy Evans) wrote:

In CRuby, it's not significantly more complicated. The callee may need to add the literal keywords to a hash anyway (if callee uses a keyword splat). Moving that hash to the last positional argument is very little work.

It's still a condition in the callee which needs to handle both kinds of callers: passing a Hash and passing literal keywords, with completely different ways to read keyword arguments from that.
That's likely rather inefficient if it happens in practice.

That's the one I'm concerned about, it means every foo(some arguments) with def foo(*args); bar(*args); end will have to check if the last positional argument is a Hash with the flag set:
args.size >= 1 && Hash === args[-1] && flagged?(args[-1]). That's a quite a few branches and it's not just reading one bit from memory, it's at least 4 memory loads (size, [-1], .class, .flag).
I think this is another motivation for having ruby2_keywords only in Ruby 2.7, as it's clearly not free for performance.

It's not free in CRuby. However, if you look at all of the surrounding code, I don't think the performance difference in CRuby will be significant.

Maybe not in CRuby. But with a sufficiently optimized Ruby implementation, arguments handling should be free whenever passed from caller to inlined callee, and should be cheap when not inlined.

A sufficiently advanced compiler in a more optimized Ruby implementation should be able to determine in most cases whether the method is ever called with such a hash, and optimize the check out, correct? :)

I don't think so. As soon as one case stores a flagged Hash we'd have to check all call sites using a *rest argument and no kwargs.
ActionDispatch already has such a case.

I think that check can only be optimized out if we create the Hash in the same compilation unit. If not, we'll have to check the flag, on every such call.
I think you need to believe my word here that this is not free and this is significant enough that we don't want to shoot ourselves for all future Ruby versions with that overhead.
For me, it ruins a good part of the performance advantages of separating positional and keyword arguments, because once again there is automatic conversion and "keywords split out from the last Array argument".
And that is not cheap, it's a pretty ugly merge of control flow and changing which arguments go where if both cases occur. And even if they don't there is the extra check.

It'd be interesting to implement this in TruffleRuby to measure, but there is no way this will happen before preview2.

Handling more involved cases where the arguments are stored and can escape may require checking all call sites with splats and no explicit keywords, though.

Yes, exactly and that is the problem.
If we have a compatibility workaround with an overhead, at least let's localize it and not affect all call sites with a rest argument (and without kwargs).

I assume in your example, initialize should accept *args and not args, as otherwise, you can't use ruby2_keywords.

Then ruby2_keywords needs to be moved to the 3 callers of build_middleware in
https://github.com/rails/rails/blob/1811e841166198bf86ae6de18d0971df77b932b4/actionpack/lib/action_dispatch/middleware/stack.rb#L92-L122
then.

It seems odd to recommend we add another feature (Kernel#send_keyword_hash) to compliment a feature (Module#ruby2_keywords) that you are strongly recommending we remove in Ruby 3.0.

I think it's a straightforward way to mark exactly which call sites need the legacy conversion behavior.
There are likely rather few of those, and it seems worth to avoid degrading the performance of all other call sites with a *rest argument and no kwargs.

Updated by jeremyevans0 (Jeremy Evans) 30 days ago

Eregon (Benoit Daloze) wrote:

jeremyevans0 (Jeremy Evans) wrote:

In CRuby, it's not significantly more complicated. The callee may need to add the literal keywords to a hash anyway (if callee uses a keyword splat). Moving that hash to the last positional argument is very little work.

It's still a condition in the callee which needs to handle both kinds of callers: passing a Hash and passing literal keywords, with completely different ways to read keyword arguments from that.
That's likely rather inefficient if it happens in practice.

As I stated last time, it's not that inefficient in CRuby. I'll try to benchmark to compare it tomorrow, but I expect it would make only a small difference in a microbenchmark, and probably no significant difference in a real world benchmark.

A sufficiently advanced compiler in a more optimized Ruby implementation should be able to determine in most cases whether the method is ever called with such a hash, and optimize the check out, correct? :)

I don't think so. As soon as one case stores a flagged Hash we'd have to check all call sites using a *rest argument and no kwargs.
ActionDispatch already has such a case.

Agreed.

I think that check can only be optimized out if we create the Hash in the same compilation unit. If not, we'll have to check the flag, on every such call.
I think you need to believe my word here that this is not free and this is significant enough that we don't want to shoot ourselves for all future Ruby versions with that overhead.

Sorry, but I won't take your word as to the extent of the performance difference. You shouldn't even take your word, as you haven't implemented it yet. I'll post a benchmark, and I ask you to do the same.

For me, it ruins a good part of the performance advantages of separating positional and keyword arguments, because once again there is automatic conversion and "keywords split out from the last Array argument".

Separating positional and keyword arguments was not done for performance reasons, it was done to avoid the issues that not separating them caused.

And that is not cheap, it's a pretty ugly merge of control flow and changing which arguments go where if both cases occur. And even if they don't there is the extra check.

Again, please post a benchmark so we can discuss actual and not theoretical performance differences.

It'd be interesting to implement this in TruffleRuby to measure, but there is no way this will happen before preview2.

Considering preview2 was released before this was posted, I agree. :)

Handling more involved cases where the arguments are stored and can escape may require checking all call sites with splats and no explicit keywords, though.

Yes, exactly and that is the problem.
If we have a compatibility workaround with an overhead, at least let's localize it and not affect all call sites with a rest argument (and without kwargs).

The initial proposal for ruby2_keywords used a VM frame flag and not a hash object flag, and was thus localized. Unfortunately, it didn't handle cases that people wanted it to handle. I think Ruby's goal of programmer happiness should outweigh minor theoretical performance issues.

I assume in your example, initialize should accept *args and not args, as otherwise, you can't use ruby2_keywords.

Then ruby2_keywords needs to be moved to the 3 callers of build_middleware in
https://github.com/rails/rails/blob/1811e841166198bf86ae6de18d0971df77b932b4/actionpack/lib/action_dispatch/middleware/stack.rb#L92-L122
then.

Correct.

It seems odd to recommend we add another feature (Kernel#send_keyword_hash) to compliment a feature (Module#ruby2_keywords) that you are strongly recommending we remove in Ruby 3.0.

I think it's a straightforward way to mark exactly which call sites need the legacy conversion behavior.
There are likely rather few of those, and it seems worth to avoid degrading the performance of all other call sites with a *rest argument and no kwargs.

Kernel#send_keyword_hash requires changing the method definition or adding a separate method definition, when one of the main benefits of ruby2_keywords is that you do not need to modify an existing method definition.

Updated by mame (Yusuke Endoh) 29 days ago

NuriYuri (Youri Nouri) Thank you for your comment, but this ticket is not a good place to discuss it. Please add it to #14183 or create a new ticket.

Updated by Eregon (Benoit Daloze) 29 days ago

jeremyevans0 (Jeremy Evans) wrote:

You shouldn't even take your word, as you haven't implemented it yet. I'll post a benchmark, and I ask you to do the same.

Fair enough, I'll try to benchmark this soon in TruffleRuby then.
If I can have a call with you it would be helpful to discuss the conceptual implementation for that logic and what examples are good to compare (and ideas related to delegation).

For me, it ruins a good part of the performance advantages of separating positional and keyword arguments, because once again there is automatic conversion and "keywords split out from the last Array argument".

Separating positional and keyword arguments was not done for performance reasons, it was done to avoid the issues that not separating them caused.

I know, but I think separating arguments can be an opportunity for better performance performance and cheaper method/block calls, and this partially removes that opportunity.

Also on the semantics level, I think it would be cleaner if there are no ways to automatically convert a positional Hash to kwargs or vice-versa.
Don't you agree?

By having ruby2_keywords in Ruby 3, we're probably going to see some abuse of it, and some confusion about it.

If the goal is to separate positional and keyword arguments, why are we leaving a backdoor?

The initial proposal for ruby2_keywords used a VM frame flag and not a hash object flag, and was thus localized. Unfortunately, it didn't handle cases that people wanted it to handle. I think Ruby's goal of programmer happiness should outweigh minor theoretical performance issues.

Yes the frame flag approach was better performance-wise in that it would only affect a known small set of call sites.

I think it's a straightforward way to mark exactly which call sites need the legacy conversion behavior.
There are likely rather few of those, and it seems worth to avoid degrading the performance of all other call sites with a *rest argument and no kwargs.

Kernel#send_keyword_hash requires changing the method definition or adding a separate method definition, when one of the main benefits of ruby2_keywords is that you do not need to modify an existing method definition.

Yes, it requires changing the method definition, but in a fairly obvious way.
I would argue everyone changing a delegation method knows very well which is the delegating call site, and marking it with send_keyword_hash is completely trivial.

I'd think it's even useful to mark and correspondingly to not mark some call sites which should not pass kwargs.

The current logic where Array#dup dup's the last argument if it's a tagged Hash (6081ddd6e6f2297862b3c7e898d28a76b8f9240b)
seems a particularly not nice workaround for the fact a tagged Hash might be used as kwargs multiple times.
#send_keyword_hash would solve this in a more clean, explicit and clear way.

I don't think there is much of a difference to add ruby2_keywords, a "visibility modifier" before def vs modifying a call site.
Both need modifications of the file.

Updated by jeremyevans0 (Jeremy Evans) 28 days ago

Eregon (Benoit Daloze) wrote:

jeremyevans0 (Jeremy Evans) wrote:

You shouldn't even take your word, as you haven't implemented it yet. I'll post a benchmark, and I ask you to do the same.

Fair enough, I'll try to benchmark this soon in TruffleRuby then.

As we discussed, here is a link to the benchmark: https://pastebin.com/raw/inwr6GvW

You'll want to compare the master branch with a branch that removes ruby2_keywords. When I ran this yesterday (before some ruby2_keywords changes were merged today), this was the diff I used for for removal: https://pastebin.com/raw/izk2qesi . This isn't a complete removal, just a removal of the code from the hot paths so we can see the performance difference.

On CRuby master branch, in the worst possible case I could design, the difference was about 1%.

Note that ruby2_keywords approach for delegation is over twice as fast as explicit keyword delegation due to reduced object allocation (see below). If you have 300 method(*args) calls for every 1 call to a method that uses ruby2_keywords for delegation instead of explicit keyword delegation, keeping ruby2_keywords still improves performance.

For me, it ruins a good part of the performance advantages of separating positional and keyword arguments, because once again there is automatic conversion and "keywords split out from the last Array argument".

Separating positional and keyword arguments was not done for performance reasons, it was done to avoid the issues that not separating them caused.

I know, but I think separating arguments can be an opportunity for better performance performance and cheaper method/block calls, and this partially removes that opportunity.

In CRuby, ruby2_keywords actually results in better performance. It is over twice as fast as explicit delegation using keywords. Here's a benchmark showing that: https://pastebin.com/raw/TNFJJSCs. So in CRuby, ruby2_keywords in general increases performance, not decreases it.

Argument delegation using ... now uses ruby2_keywords internally. This was done to avoid warnings and not for performance reasons, but it does increase performance substantially.

Also on the semantics level, I think it would be cleaner if there are no ways to automatically convert a positional Hash to kwargs or vice-versa.
Don't you agree?

Technically, **hash automatically converts a positional hash to keywords. So I definitely think we want a way to automatically convert from positional hash to keywords, otherwise all keyword arguments would need to be specified explicitly in every call.

By having ruby2_keywords in Ruby 3, we're probably going to see some abuse of it, and some confusion about it.

I don't think we'll see major problems due to it, but my precognition has never been 100%.

If the goal is to separate positional and keyword arguments, why are we leaving a backdoor?

This isn't a backdoor, it's a tunnel. It allows you to tunnel keyword arguments through a method that doesn't explicitly list itself as accepting keyword arguments. It doesn't have any of the issues that caused us to separate positional and keyword arguments. For the following code:

  ruby2_keywords def foo(*args, &block)
    bar(*args)
  end

Method bar knows that it is passed a positional hash if you call foo(h), and knows it is passed keywords if you call foo(**h).

Stating that ruby2_keywords violates or backtracks on keyword argument separation is wrong.

I think it's a straightforward way to mark exactly which call sites need the legacy conversion behavior.
There are likely rather few of those, and it seems worth to avoid degrading the performance of all other call sites with a *rest argument and no kwargs.

Kernel#send_keyword_hash requires changing the method definition or adding a separate method definition, when one of the main benefits of ruby2_keywords is that you do not need to modify an existing method definition.

Yes, it requires changing the method definition, but in a fairly obvious way.
I would argue everyone changing a delegation method knows very well which is the delegating call site, and marking it with send_keyword_hash is completely trivial.

This would require adding a method to Kernel in addition to the ruby2_keywords method added to Module. It would also make the method slower on older versions of Ruby (and possibly also the current version of Ruby), since they would not be able to use the optimized version of send.

I'd think it's even useful to mark and correspondingly to not mark some call sites which should not pass kwargs.

The current logic where Array#dup dup's the last argument if it's a tagged Hash (6081ddd6e6f2297862b3c7e898d28a76b8f9240b)
seems a particularly not nice workaround for the fact a tagged Hash might be used as kwargs multiple times.
#send_keyword_hash would solve this in a more clean, explicit and clear way.

The commit referenced is not related to Array#dup. It calls rb_hash_dup on the last element, because that is the equivalent of what **hash does. send_keyword_hash would need to dup the hash in the exact same way, otherwise the callee could modify the hash and have the changes reflected in the caller, which is not how keyword hashes are supposed to work.

I don't think there is much of a difference to add ruby2_keywords, a "visibility modifier" before def vs modifying a call site.
Both need modifications of the file.

Note that you cannot have send_keyword_hash without ruby2_keywords. At the very least, you are at least doubling the work required to get correct delegation if you add it.

Your arguments against ruby2_keywords in general boil down to these things:

  • It is bad for performance. I have proven this not to be true in CRuby, and we are not yet sure the extent to which it affects other implementations. On CRuby, ruby2_keywords actually helps performance significantly due to fewer object allocations during delegation.

  • Users will have to modify their code twice. First, this is only true if ruby2_keywords is removed. Removal has not been decided on, yet, though the reason it was renamed from pass_keywords to ruby2_keywords was because it was expected to be removed after Ruby 2.6 EOL. Second, your proposal for duplicate method definitions with a version check still would result in almost all users modifying their code twice, as almost no users want to have dead code in their codebase for ruby versions they no longer support. So there is no practical difference in the number of times users will need to modify their code, even if ruby2_keywords is removed at some future point.

  • It's not semantic, going against keyword argument separation. As I explained above, it does not go against keyword argument separation, it works with it to handle delegation in a backwards compatible manner.

  • It looks strange, and will look more strange in Ruby 4+. I kind of agree. I think the pass_keywords name was better. This doesn't seem like a very strong reason, though.

Updated by Eregon (Benoit Daloze) 25 days ago

I want to expand on my semantics concern, for the performance concern I should get some numbers first.

jeremyevans0 (Jeremy Evans) wrote:

Technically, **hash automatically converts a positional hash to keywords. So I definitely think we want a way to automatically convert from positional hash to keywords, otherwise all keyword arguments would need to be specified explicitly in every call.

Yes of course. What I meant is ruby2_keywords is the only thing which allows passing keyword arguments without explicitly using ** or literal keywords (foo(a: 1)) at the call site (those 2 are explicit and fine).
For instance, bar(*args) from the code above passes keyword arguments even though no Ruby code should be able to do that in 3.0, but ruby2_keywords is an exception here.

It's not so nice that somecall(*args) behaves differently depending on whether Hash === args[-1] && tagged?(args[-1]) (which is not a property of the lexical code, but a dynamic runtime flag).
So looking at some Ruby code in some gem, I cannot know if somecall(*args) may pass or never passes keyword arguments.
For example if my code defines somecall, that might make it unclear whether I should accept keyword arguments or not.
Having an explicit send_keyword_hash would fix that and mark the only places that might take *args but actually pass keyword arguments too.

mame (Yusuke Endoh) What do you think of that specific concern?

Also available in: Atom PDF