| Age | Commit message (Collapse) | Author |
|---|
|
Previously, if an argument splat and keywords are provided by
the caller, it did not check whether the method/proc accepted
keywords before avoiding the allocation. This is incorrect,
because if the method/proc does not accept keywords, the
keywords passed by the caller are added as a positional
argument, so there must be an allocation to avoid mutating
the positional splat argument.
Add a check that if the caller passes keywords, the
method/proc must accept keywords in order to optimize.
If the caller passes a keyword splat, either the
method/proc must accept keywords, or the keyword splat must
be empty in order to optimize.
If keywords are explicitly disallowed via `**nil`, the
optimization should be skipped, because the array is mutated
before the ArgumentError exception is raised.
In addition to a test for the correct behavior, add an
allocation test for a method that accepts an anonymous splat
without keywords.
Fixes [Bug #21757]
|
|
Fix handling of PM_CONSTANT_PATH_NODE node in keyword arguments with ARGS_SPLAT
This was handled correctly in parse.y (NODE_COLON2), but not in
prism. This wasn't caught earlier, because I only added tests for
the optimized case and not the unoptimized case. Add tests for
the unoptimized case.
In code terms:
```ruby
m(*a, kw: lvar::X) # Does not require allocation for *a
m(*a, kw: method()::X) # Requires allocation for *a
```
This commit fixes the second case when prism is used.
|
|
Previously, proc calls such as:
```ruby
proc{|| }.(**empty_hash)
proc{|b: 1| }.(**r2k_array_with_empty_hash)
```
both allocated hashes unnecessarily, due to two separate code paths.
The first call goes through CALLER_SETUP_ARG/vm_caller_setup_keyword_hash,
and is simple to fix by not duping an empty keyword hash that will be
dropped.
The second case is more involved, in setup_parameters_complex, but is
fixed the exact same way as when the ruby2_keywords hash is not empty,
by flattening the rest array to the VM stack, ignoring the last
element (the empty keyword splat). Add a flatten_rest_array static
function to handle this case.
Update test_allocation.rb to automatically convert the method call
allocation tests to proc allocation tests, at least for the calls
that can be converted. With the code changes, all proc call
allocation tests pass, showing that proc calls and method calls
now allocate the same number of objects.
I've audited the allocation tests, and I believe that all of the low
hanging fruit has been collected. All remaining allocations are
either caller side:
* Positional splat + post argument
* Multiple positional splats
* Literal keywords + keyword splat
* Multiple keyword splats
Or callee side:
* Positional splat parameter
* Keyword splat parameter
* Keyword to positional argument conversion for methods that don't accept keywords
* ruby2_keywords method called with keywords
Reapplies abc04e898b627ab37fa9dd5e330f239768778d8b, which was reverted at
d56470a27c5a8a2e7aee7a76cea445c2d29c0c59, with the addition of a bug fix and
test.
Fixes [Bug #20679]
Notes:
Merged: https://github.com/ruby/ruby/pull/11409
Merged-By: jeremyevans <code@jeremyevans.net>
|
|
This reverts commit abc04e898b627ab37fa9dd5e330f239768778d8b.
This caused problems in a Rails test.
Notes:
Merged: https://github.com/ruby/ruby/pull/11394
|
|
Previous, proc calls such as:
```ruby
proc{|| }.(**empty_hash)
proc{|b: 1| }.(**r2k_array_with_empty_hash)
```
both allocated hashes unnecessarily, due to two separate code paths.
The first call goes through CALLER_SETUP_ARG/vm_caller_setup_keyword_hash,
and is simple to fix by not duping an empty keyword hash that will be
dropped.
The second case is more involved, in setup_parameters_complex, but is
fixed the exact same way as when the ruby2_keywords hash is not empty,
by flattening the rest array to the VM stack, ignoring the last
element (the empty keyword splat). Add a flatten_rest_array static
function to handle this case.
Update test_allocation.rb to automatically convert the method call
allocation tests to proc allocation tests, at least for the calls
that can be converted. With the code changes, all proc call
allocation tests pass, showing that proc calls and method calls
now allocate the same number of objects.
I've audited the allocation tests, and I believe that all of the low
hanging fruit has been collected. All remaining allocations are
either caller side:
* Positional splat + post argument
* Multiple positional splats
* Literal keywords + keyword splat
* Multiple keyword splats
Or callee side:
* Positional splat parameter
* Keyword splat parameter
* Keyword to positional argument conversion for methods that don't accept keywords
* ruby2_keywords method called with keywords
Notes:
Merged: https://github.com/ruby/ruby/pull/11258
|
|
When calling a method that does not accept a positional splat
parameter with a splatted array with a ruby2_keywords flagged hash,
there is no need to duplicate the splatted array. Previously,
Ruby would duplicate the splatted array and potentially modify
it before flattening it to the VM stack
Use a similar approach as the f(*ary, **hash) optimization,
flattening the splatted array to the VM stack without modifying
it, and make any modifications needed to the VM stack.
Notes:
Merged: https://github.com/ruby/ruby/pull/11161
|
|
When calling a method that accepts keywords but not a keyword
splat with a splatted array with a ruby2_keywords flagged hash,
there is no need to duplicate the ruby2_keywords flagged hash,
since it will be accessed to get the keyword values, but it will
not be modified.
Notes:
Merged: https://github.com/ruby/ruby/pull/11161
|
|
Notes:
Merged: https://github.com/ruby/ruby/pull/11161
|
|
This avoids an array allocation when calling a method that does
not accept a positional splat or keywords with both a positional
splat and keywords. Previously, Ruby would dup the positional
splat to append the keyword splat to it. Then it would flatten
the dupped positional splat array to the VM stack.
This flattens the given positional splat to the VM stack, then
adds the keyword splat hash after the last positional splat
element on the VM stack, avoiding the need to modify
the positional splat array.
Notes:
Merged: https://github.com/ruby/ruby/pull/11161
|
|
a: lvar), and other calls
The `f(arg, *arg, **arg, **arg)` case was previously not optimized.
The optimizer didn't optimize this case because of the multiple
keyword splats, and the compiler didn't optimize it because the
`f(*arg, **arg, **arg)` optimization added in
0ee3960685e283d8e75149a8777eb0109d41509a didn't apply.
I found it difficult to apply this optimization without changing
the `setup_args_core` API, since by the time you get to the ARGSCAT
case, you don't know whether you were called recursively or directly,
so I'm not sure if it was possible to know at that point whether the
array allocation could be avoided.
This changes the dup_rest argument in `setup_args_core` from an int
to a pointer to int. This allows us to track whether we have allocated
a caller side array for multiple splats or splat+post across
recursive calls. Check the pointed value (*dup_rest) to determine the
`splatarray` argument. If dup_rest is 1, then use `splatarray true`
(caller-side array allocation), then set *dup_rest back to 0, ensuring
only a single `splatarray true` per method call.
Before calling `setup_args_core`, check whether the array allocation
can be avoided safely using `splatarray false`. Optimizable cases are:
```
// f(*arg)
SPLAT
// f(1, *arg)
ARGSCAT
LIST
// f(*arg, **arg)
ARGSPUSH
SPLAT
HASH nd_brace=0
// f(1, *arg, **arg)
ARGSPUSH
ARGSCAT
LIST
HASH nd_brace=0
```
If so, dup_rest is set to 0 instead of 1 to avoid the allocation.
After calling `setup_args_core`, check the flag. If the flag
includes `VM_CALL_ARGS_SPLAT`, and the pointed value has changed,
indicating `splatarray true` was used, then also set
`VM_CALL_ARGS_SPLAT_MUT` in the flag.
My initial attempt at this broke the `f(*ary, &ary.pop)` test,
because we were not duplicating the ary in the splat even though
it was modified later (evaluation order issue). The initial attempt
would also break `f(*ary, **ary.pop)` or `f(*ary, kw: ary.pop)` cases
for the same reason. I added test cases for those evaluation
order issues.
Add setup_args_dup_rest_p static function that checks that a given
node is safe. Call that on the block pass node to determine if
the block pass node is safe. Also call it on each of the hash
key/value nodes to test that they are safe. If any are not safe,
then set dup_rest = 1 so that `splatarray true` will be used to
avoid the evaluation order issue.
This new approach has the affect of optimizing most cases of
literal keywords after positional splats. Previously, only
static keyword hashes after positional splats avoided array
allocation for the splat. Now, most dynamic keyword hashes
after positional splats also avoid array allocation.
Add allocation tests for dynamic keyword keyword hashes after
positional splats.
setup_args_dup_rest_p is currently fairly conservative. It
could definitely be expanded to handle additional node types
to reduce allocations in additional cases.
Notes:
Merged: https://github.com/ruby/ruby/pull/11161
|
|
ruby2_keywords method
Treat this similar to keyword splatting nil, using goto ignore.
However, keep previous behavior if the method accepts a keyword
splat, to avoid double hash allocation.
This also can avoid an array allocation when calling a method
that doesn't have any splat parameters but supports literal
keyword parameters, because ignore_keyword_hash_p was not
ignoring the keyword hash in that case.
This change doesn't remove the empty ruby2_keywords hash from
the array, which caused an assertion failure if the method
being called accepted keywords in some cases. Modify the
assertion to handle this case. An alternative approach would
add a flag to the args struct so the args_argc calculation could
handle this case and report the correct argc, but such an approach
would likely be slower.
|
|
For calls such as:
m(*ary, a: 2, **h)
m(*ary, **h, **h, **h)
Where m does not take a positional argument splat, there was previously
an array allocation (splatarray true) to dup ary, even though it was not
necessary to do so. This is because the elimination of the array allocation
(splatarray false) was performed in the optimizer, and the optimizer didn't
handle this case, because the instructions for the keywords can be of
arbitrary length.
Move part of the optimization from the optimizer to the compiler,
detecting parse trees of the form:
ARGS_PUSH:
head: SPLAT
tail: HASH (without brace)
And using splatarray false instead of splatarray true for them.
Unfortunately, moving part of the optimization to the compiler broke
the hash allocation elimination optimization for calls of the
form:
m(*ary, a: 2)
That's because the compiler had already set splatarray false,
and the optimizer code was looking for splatarray true.
Split the array allocation elimination and hash allocation
elimination in the optimizer so that the hash allocation
elimination will still apply if the compiler performs the
splatarray false optimization.
|
|
|
|
This patch optimizes forwarding callers and callees. It only optimizes methods that only take `...` as their parameter, and then pass `...` to other calls.
Calls it optimizes look like this:
```ruby
def bar(a) = a
def foo(...) = bar(...) # optimized
foo(123)
```
```ruby
def bar(a) = a
def foo(...) = bar(1, 2, ...) # optimized
foo(123)
```
```ruby
def bar(*a) = a
def foo(...)
list = [1, 2]
bar(*list, ...) # optimized
end
foo(123)
```
All variants of the above but using `super` are also optimized, including a bare super like this:
```ruby
def foo(...)
super
end
```
This patch eliminates intermediate allocations made when calling methods that accept `...`.
We can observe allocation elimination like this:
```ruby
def m
x = GC.stat(:total_allocated_objects)
yield
GC.stat(:total_allocated_objects) - x
end
def bar(a) = a
def foo(...) = bar(...)
def test
m { foo(123) }
end
test
p test # allocates 1 object on master, but 0 objects with this patch
```
```ruby
def bar(a, b:) = a + b
def foo(...) = bar(...)
def test
m { foo(1, b: 2) }
end
test
p test # allocates 2 objects on master, but 0 objects with this patch
```
How does it work?
-----------------
This patch works by using a dynamic stack size when passing forwarded parameters to callees.
The caller's info object (known as the "CI") contains the stack size of the
parameters, so we pass the CI object itself as a parameter to the callee.
When forwarding parameters, the forwarding ISeq uses the caller's CI to determine how much stack to copy, then copies the caller's stack before calling the callee.
The CI at the forwarded call site is adjusted using information from the caller's CI.
I think this description is kind of confusing, so let's walk through an example with code.
```ruby
def delegatee(a, b) = a + b
def delegator(...)
delegatee(...) # CI2 (FORWARDING)
end
def caller
delegator(1, 2) # CI1 (argc: 2)
end
```
Before we call the delegator method, the stack looks like this:
```
Executing Line | Code | Stack
---------------+---------------------------------------+--------
1| def delegatee(a, b) = a + b | self
2| | 1
3| def delegator(...) | 2
4| # |
5| delegatee(...) # CI2 (FORWARDING) |
6| end |
7| |
8| def caller |
-> 9| delegator(1, 2) # CI1 (argc: 2) |
10| end |
```
The ISeq for `delegator` is tagged as "forwardable", so when `caller` calls in
to `delegator`, it writes `CI1` on to the stack as a local variable for the
`delegator` method. The `delegator` method has a special local called `...`
that holds the caller's CI object.
Here is the ISeq disasm fo `delegator`:
```
== disasm: #<ISeq:delegator@-e:1 (1,0)-(1,39)>
local table (size: 1, argc: 0 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 1] "..."@0
0000 putself ( 1)[LiCa]
0001 getlocal_WC_0 "..."@0
0003 send <calldata!mid:delegatee, argc:0, FCALL|FORWARDING>, nil
0006 leave [Re]
```
The local called `...` will contain the caller's CI: CI1.
Here is the stack when we enter `delegator`:
```
Executing Line | Code | Stack
---------------+---------------------------------------+--------
1| def delegatee(a, b) = a + b | self
2| | 1
3| def delegator(...) | 2
-> 4| # | CI1 (argc: 2)
5| delegatee(...) # CI2 (FORWARDING) | cref_or_me
6| end | specval
7| | type
8| def caller |
9| delegator(1, 2) # CI1 (argc: 2) |
10| end |
```
The CI at `delegatee` on line 5 is tagged as "FORWARDING", so it knows to
memcopy the caller's stack before calling `delegatee`. In this case, it will
memcopy self, 1, and 2 to the stack before calling `delegatee`. It knows how much
memory to copy from the caller because `CI1` contains stack size information
(argc: 2).
Before executing the `send` instruction, we push `...` on the stack. The
`send` instruction pops `...`, and because it is tagged with `FORWARDING`, it
knows to memcopy (using the information in the CI it just popped):
```
== disasm: #<ISeq:delegator@-e:1 (1,0)-(1,39)>
local table (size: 1, argc: 0 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 1] "..."@0
0000 putself ( 1)[LiCa]
0001 getlocal_WC_0 "..."@0
0003 send <calldata!mid:delegatee, argc:0, FCALL|FORWARDING>, nil
0006 leave [Re]
```
Instruction 001 puts the caller's CI on the stack. `send` is tagged with
FORWARDING, so it reads the CI and _copies_ the callers stack to this stack:
```
Executing Line | Code | Stack
---------------+---------------------------------------+--------
1| def delegatee(a, b) = a + b | self
2| | 1
3| def delegator(...) | 2
4| # | CI1 (argc: 2)
-> 5| delegatee(...) # CI2 (FORWARDING) | cref_or_me
6| end | specval
7| | type
8| def caller | self
9| delegator(1, 2) # CI1 (argc: 2) | 1
10| end | 2
```
The "FORWARDING" call site combines information from CI1 with CI2 in order
to support passing other values in addition to the `...` value, as well as
perfectly forward splat args, kwargs, etc.
Since we're able to copy the stack from `caller` in to `delegator`'s stack, we
can avoid allocating objects.
I want to do this to eliminate object allocations for delegate methods.
My long term goal is to implement `Class#new` in Ruby and it uses `...`.
I was able to implement `Class#new` in Ruby
[here](https://github.com/ruby/ruby/pull/9289).
If we adopt the technique in this patch, then we can optimize allocating
objects that take keyword parameters for `initialize`.
For example, this code will allocate 2 objects: one for `SomeObject`, and one
for the kwargs:
```ruby
SomeObject.new(foo: 1)
```
If we combine this technique, plus implement `Class#new` in Ruby, then we can
reduce allocations for this common operation.
Co-Authored-By: John Hawthorn <john@hawthorn.email>
Co-Authored-By: Alan Wu <XrXr@users.noreply.github.com>
|
|
If the method being called does not have a positional splat
parameter, there is no point in allocating the array, as
decrementing given_argc is sufficient to ensure the empty keyword
hash is not considered an argument, assuming that we are calling
a method/lambda and not a regular proc.
|
|
If the method being called does not have a keyword splat parameter,
there is no point in allocating the hash, because the hash will
be unused (as empty keyword hashes are ignored).
|
|
This tests ruby2_keywords flagged methods, as well as passing
ruby2_keywords flagged hashes to other methods.
Some of the behavior here is questionable, such as allocating
different numbers of objects depending on whether a block is
passed or whether YJIT is enabled. I think there are likely ways
to eliminate allocations in certain cases. However, this gives
us a baseline and shows us where it is possible to make
improvements.
|
|
These are designed to prevent allocation regressions (commits that
increase the number of implicitly allocated arrays and hashes). We
have already had three commits in the last couple weeks to fix
allocation regressions:
* 15dc3aaa311b32203d8ffb414bcf9b8e55ce5691
* aceee71c35e0b387691836e756b4e008efd84cf1
* c38878494377c94f2425a81e598260ea944ef7f3
This test suite should hopefully allow us to find such regressions
in CI before commit, to avoid committing future allocation regressions.
This uses assert_separately around each set of tests. Doing it for
each individual check was too slow. Failures are gathered and
reported at the end of the the suite as a single assertion, with
the message describing all failures.
|
