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


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.

----------------------------------------
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-82258

* 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/

Unsubscribe: <mailto:ruby-core-request / ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>