[ruby-core:95535] [Ruby master Misc#16188] What are the performance implications of the new keyword arguments in 2.7 and 3.0?

[merch-redmine@jeremyevans.net]

Issue #16188 has been updated by jeremyevans0 (Jeremy Evans).


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:

```ruby
  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.

----------------------------------------
Misc #16188: What are the performance implications of the new keyword arguments in 2.7 and 3.0?
https://bugs.ruby-lang.org/issues/16188#change-82311

* Author: Eregon (Benoit Daloze)
* Status: Open
* Priority: Normal
* Assignee: jeremyevans0 (Jeremy Evans)
----------------------------------------
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?



-- 
https://bugs.ruby-lang.org/