Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CRuby test failure: TestAllocation #2994

Closed
15 tasks done
kddnewton opened this issue Aug 21, 2024 · 1 comment · Fixed by ruby/ruby#11484
Closed
15 tasks done

CRuby test failure: TestAllocation #2994

kddnewton opened this issue Aug 21, 2024 · 1 comment · Fixed by ruby/ruby#11484
Assignees
@XrXr
Labels
bug Something isn't working compiler
Milestone
CRuby unblocked

Comments

@kddnewton
Copy link
Collaborator

kddnewton commented Aug 21, 2024 •
edited
Loading

We need to implement all of the same recent optimizations to setup_args that were added in 3.4. This largely involves avoiding array and hash allocations. This is done through checking for specific patterns and using a combination of flags and instructions like pushtoarray and concattoarray.

  • TestAllocation::MethodCall#test_anonymous_splat_and_anonymous_keyword_splat_parameters
  • TestAllocation::MethodCall#test_argument_forwarding
  • TestAllocation::MethodCall#test_nested_anonymous_splat_and_anonymous_keyword_splat_parameters
  • TestAllocation::MethodCall#test_nested_argument_forwarding
  • TestAllocation::MethodCall#test_optional_parameter
  • TestAllocation::MethodCall#test_positional_splat_and_keyword_parameter
  • TestAllocation::MethodCall#test_required_and_keyword_splat_parameter
  • TestAllocation::MethodCall#test_required_parameter
  • TestAllocation::MethodCall#test_required_positional_and_keyword_parameter
  • TestAllocation::ProcCall#test_anonymous_splat_and_anonymous_keyword_splat_parameters
  • TestAllocation::ProcCall#test_optional_parameter
  • TestAllocation::ProcCall#test_positional_splat_and_keyword_parameter
  • TestAllocation::ProcCall#test_required_and_keyword_splat_parameter
  • TestAllocation::ProcCall#test_required_parameter
  • TestAllocation::ProcCall#test_required_positional_and_keyword_parameter

In order to test changes, you will need to apply to following diff to ruby/ruby (copy into a file like prism.patch and run git apply prism.patch):

diff --git a/internal/parse.h b/internal/parse.h
index fdf2a10183..88230fd177 100644
--- a/internal/parse.h
+++ b/internal/parse.h
@@ -16,7 +16,7 @@
 // 0: parse.y
 // 1: Prism
 #ifndef RB_DEFAULT_PARSER
-#define RB_DEFAULT_PARSER 0
+#define RB_DEFAULT_PARSER 1
 #endif
 
 #ifdef UNIVERSAL_PARSER

Then, to run the individual tests, run:

make -j
make test-all TESTS=test/ruby/test_allocation.rb TESTOPTS='-n /test_no_parameters/'
@kddnewton kddnewton added bug Something isn't working compiler labels Aug 21, 2024
@kddnewton kddnewton added this to the CRuby unblocked milestone Aug 21, 2024
@kddnewton
Copy link
Collaborator Author

@eileencodes assigning you only to make it clear that you're working on these, not to indicate that you're responsible for all of them

eileencodes added a commit to eileencodes/ruby that referenced this issue Aug 22, 2024
@eileencodes @kddnewton
[PRISM] Fix allocations for keyword splat params
7332434 
Fixes the following allocations tests:

* `test_keyword_and_keyword_splat_parameter`
* `test_keyword_parameter`
* `test_keyword_splat_parameter`
* `test_no_array_allocation_with_splat_and_nonstatic_keywords`
* `test_no_parameters`
* `test_positional_splat_and_keyword_splat_parameter`
* `test_ruby2_keywords`

* Checks for `first_chunk` and if `stack_length == 0` to match the
upstream parser. Otherwise, this optimization is skipped.
* Subtracts the index, otherwise it will skip the hash allocation for
the following: `keyword(*empty_array, a: 2, **empty_hash)`.
* Sets `dup_rest` in order to determine when to set the correct flags
* Doesn't set `VM_CALL_KW_SPLAT_MUT` flag unless `dup_rest` doesn't
match `initial_dup_rest`.

Given the following code:

```ruby
keyword(*empty_array, a: 2)
```

Instructions before:

```
== disasm: #<ISeq:[email protected]:4 (4,0)-(8,3)>
local table (size: 2, argc: 1 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 2] empty_hash@0<Arg>[ 1] empty_array@1
0000 newarray                               0                         (   5)[LiCa]
0002 setlocal_WC_0                          empty_array@1
0004 putself                                                          (   7)[Li]
0005 getlocal_WC_0                          empty_array@1
0007 splatarray                             true
0009 putobject                              :a
0011 putobject                              2
0013 newhash                                2
0015 opt_send_without_block                 <calldata!mid:keyword, argc:2, ARGS_SPLAT|ARGS_SPLAT_MUT|FCALL|KW_SPLAT>
0017 leave                                                            (   8)[Re]
```

Instructions after:

```
== disasm: #<ISeq:[email protected]:4 (4,0)-(8,3)>
local table (size: 2, argc: 1 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 2] empty_hash@0<Arg>[ 1] empty_array@1
0000 newarray                               0                         (   5)[LiCa]
0002 setlocal_WC_0                          empty_array@1
0004 putself                                                          (   7)[Li]
0005 getlocal_WC_0                          empty_array@1
0007 splatarray                             false
0009 putobject                              {:a=>2}
0011 opt_send_without_block                 <calldata!mid:keyword, argc:2, ARGS_SPLAT|FCALL|KW_SPLAT>
0013 leave                                                            (   8)[Re]
```

Differences:

* `splatarray` is `false` not `true
* `putobject`, `putobject`, `newhash` is simply `putobject` with
optimizations on
* Remove `ARGS_SPLAT_MUT` flag

Related: ruby/prism#2994

Co-authored-by: Kevin Newton <[email protected]>
@eileencodes eileencodes mentioned this issue Aug 22, 2024
Merged
eileencodes added a commit to eileencodes/ruby that referenced this issue Aug 23, 2024
@eileencodes @kddnewton
[PRISM] Fix allocations for keyword splat params
c087422 
Fixes the following allocations tests:

* `test_keyword_and_keyword_splat_parameter`
* `test_keyword_parameter`
* `test_keyword_splat_parameter`
* `test_no_array_allocation_with_splat_and_nonstatic_keywords`
* `test_no_parameters`
* `test_positional_splat_and_keyword_splat_parameter`
* `test_ruby2_keywords`

* Checks for `first_chunk` and if `stack_length == 0` to match the
upstream parser. Otherwise, this optimization is skipped.
* Subtracts the index, otherwise it will skip the hash allocation for
the following: `keyword(*empty_array, a: 2, **empty_hash)`.
* Sets `dup_rest` in order to determine when to set the correct flags
* Doesn't set `VM_CALL_KW_SPLAT_MUT` flag unless `dup_rest` doesn't
match `initial_dup_rest`.

Given the following code:

```ruby
keyword(*empty_array, a: 2)
```

Instructions before:

```
== disasm: #<ISeq:[email protected]:4 (4,0)-(8,3)>
local table (size: 2, argc: 1 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 2] empty_hash@0<Arg>[ 1] empty_array@1
0000 newarray                               0                         (   5)[LiCa]
0002 setlocal_WC_0                          empty_array@1
0004 putself                                                          (   7)[Li]
0005 getlocal_WC_0                          empty_array@1
0007 splatarray                             true
0009 putobject                              :a
0011 putobject                              2
0013 newhash                                2
0015 opt_send_without_block                 <calldata!mid:keyword, argc:2, ARGS_SPLAT|ARGS_SPLAT_MUT|FCALL|KW_SPLAT>
0017 leave                                                            (   8)[Re]
```

Instructions after:

```
== disasm: #<ISeq:[email protected]:4 (4,0)-(8,3)>
local table (size: 2, argc: 1 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 2] empty_hash@0<Arg>[ 1] empty_array@1
0000 newarray                               0                         (   5)[LiCa]
0002 setlocal_WC_0                          empty_array@1
0004 putself                                                          (   7)[Li]
0005 getlocal_WC_0                          empty_array@1
0007 splatarray                             false
0009 putobject                              {:a=>2}
0011 opt_send_without_block                 <calldata!mid:keyword, argc:2, ARGS_SPLAT|FCALL|KW_SPLAT>
0013 leave                                                            (   8)[Re]
```

Differences:

* `splatarray` is `false` not `true
* `putobject`, `putobject`, `newhash` is simply `putobject` with
optimizations on
* Remove `ARGS_SPLAT_MUT` flag

Related: ruby/prism#2994

Co-authored-by: Kevin Newton <[email protected]>
eileencodes added a commit to eileencodes/ruby that referenced this issue Aug 27, 2024
@eileencodes @kddnewton
[PRISM] Fix allocations for keyword splat params
af51e58 
Fixes the following allocations tests:

* `test_keyword_and_keyword_splat_parameter`
* `test_keyword_parameter`
* `test_keyword_splat_parameter`
* `test_no_array_allocation_with_splat_and_nonstatic_keywords`
* `test_no_parameters`
* `test_positional_splat_and_keyword_splat_parameter`
* `test_ruby2_keywords`

* Checks for `first_chunk` and if `stack_length == 0` to match the
upstream parser. Otherwise, this optimization is skipped.
* Subtracts the index, otherwise it will skip the hash allocation for
the following: `keyword(*empty_array, a: 2, **empty_hash)`.
* Sets `dup_rest` in order to determine when to set the correct flags
* Doesn't set `VM_CALL_KW_SPLAT_MUT` flag unless `dup_rest` doesn't
match `initial_dup_rest`.

Given the following code:

```ruby
keyword(*empty_array, a: 2)
```

Instructions before:

```
== disasm: #<ISeq:[email protected]:4 (4,0)-(8,3)>
local table (size: 2, argc: 1 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 2] empty_hash@0<Arg>[ 1] empty_array@1
0000 newarray                               0                         (   5)[LiCa]
0002 setlocal_WC_0                          empty_array@1
0004 putself                                                          (   7)[Li]
0005 getlocal_WC_0                          empty_array@1
0007 splatarray                             true
0009 putobject                              :a
0011 putobject                              2
0013 newhash                                2
0015 opt_send_without_block                 <calldata!mid:keyword, argc:2, ARGS_SPLAT|ARGS_SPLAT_MUT|FCALL|KW_SPLAT>
0017 leave                                                            (   8)[Re]
```

Instructions after:

```
== disasm: #<ISeq:[email protected]:4 (4,0)-(8,3)>
local table (size: 2, argc: 1 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 2] empty_hash@0<Arg>[ 1] empty_array@1
0000 newarray                               0                         (   5)[LiCa]
0002 setlocal_WC_0                          empty_array@1
0004 putself                                                          (   7)[Li]
0005 getlocal_WC_0                          empty_array@1
0007 splatarray                             false
0009 putobject                              {:a=>2}
0011 opt_send_without_block                 <calldata!mid:keyword, argc:2, ARGS_SPLAT|FCALL|KW_SPLAT>
0013 leave                                                            (   8)[Re]
```

Differences:

* `splatarray` is `false` not `true
* `putobject`, `putobject`, `newhash` is simply `putobject` with
optimizations on
* Remove `ARGS_SPLAT_MUT` flag

Related: ruby/prism#2994

Co-authored-by: Kevin Newton <[email protected]>
eileencodes added a commit to eileencodes/ruby that referenced this issue Aug 27, 2024
@eileencodes @kddnewton
[PRISM] Fix allocations for keyword splat params
254614b 
Fixes the following allocations tests:

* `test_keyword_and_keyword_splat_parameter`
* `test_keyword_parameter`
* `test_keyword_splat_parameter`
* `test_no_array_allocation_with_splat_and_nonstatic_keywords`
* `test_no_parameters`
* `test_positional_splat_and_keyword_splat_parameter`
* `test_ruby2_keywords`

* Checks for `first_chunk` and if `stack_length == 0` to match the
upstream parser. Otherwise, this optimization is skipped.
* Subtracts the index, otherwise it will skip the hash allocation for
the following: `keyword(*empty_array, a: 2, **empty_hash)`.
* Sets `dup_rest` in order to determine when to set the correct flags
* Doesn't set `VM_CALL_KW_SPLAT_MUT` flag unless `dup_rest` doesn't
match `initial_dup_rest`.

Given the following code:

```ruby
keyword(*empty_array, a: 2)
```

Instructions before:

```
== disasm: #<ISeq:[email protected]:4 (4,0)-(8,3)>
local table (size: 2, argc: 1 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 2] empty_hash@0<Arg>[ 1] empty_array@1
0000 newarray                               0                         (   5)[LiCa]
0002 setlocal_WC_0                          empty_array@1
0004 putself                                                          (   7)[Li]
0005 getlocal_WC_0                          empty_array@1
0007 splatarray                             true
0009 putobject                              :a
0011 putobject                              2
0013 newhash                                2
0015 opt_send_without_block                 <calldata!mid:keyword, argc:2, ARGS_SPLAT|ARGS_SPLAT_MUT|FCALL|KW_SPLAT>
0017 leave                                                            (   8)[Re]
```

Instructions after:

```
== disasm: #<ISeq:[email protected]:4 (4,0)-(8,3)>
local table (size: 2, argc: 1 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 2] empty_hash@0<Arg>[ 1] empty_array@1
0000 newarray                               0                         (   5)[LiCa]
0002 setlocal_WC_0                          empty_array@1
0004 putself                                                          (   7)[Li]
0005 getlocal_WC_0                          empty_array@1
0007 splatarray                             false
0009 putobject                              {:a=>2}
0011 opt_send_without_block                 <calldata!mid:keyword, argc:2, ARGS_SPLAT|FCALL|KW_SPLAT>
0013 leave                                                            (   8)[Re]
```

Differences:

* `splatarray` is `false` not `true
* `putobject`, `putobject`, `newhash` is simply `putobject` with
optimizations on
* Remove `ARGS_SPLAT_MUT` flag

Related: ruby/prism#2994

Co-authored-by: Kevin Newton <[email protected]>
kddnewton added a commit to ruby/ruby that referenced this issue Aug 27, 2024
@eileencodes @kddnewton
[PRISM] Fix allocations for keyword splat params
7462cc7 
Fixes the following allocations tests:

* `test_keyword_and_keyword_splat_parameter`
* `test_keyword_parameter`
* `test_keyword_splat_parameter`
* `test_no_array_allocation_with_splat_and_nonstatic_keywords`
* `test_no_parameters`
* `test_positional_splat_and_keyword_splat_parameter`
* `test_ruby2_keywords`

* Checks for `first_chunk` and if `stack_length == 0` to match the
upstream parser. Otherwise, this optimization is skipped.
* Subtracts the index, otherwise it will skip the hash allocation for
the following: `keyword(*empty_array, a: 2, **empty_hash)`.
* Sets `dup_rest` in order to determine when to set the correct flags
* Doesn't set `VM_CALL_KW_SPLAT_MUT` flag unless `dup_rest` doesn't
match `initial_dup_rest`.

Given the following code:

```ruby
keyword(*empty_array, a: 2)
```

Instructions before:

```
== disasm: #<ISeq:[email protected]:4 (4,0)-(8,3)>
local table (size: 2, argc: 1 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 2] empty_hash@0<Arg>[ 1] empty_array@1
0000 newarray                               0                         (   5)[LiCa]
0002 setlocal_WC_0                          empty_array@1
0004 putself                                                          (   7)[Li]
0005 getlocal_WC_0                          empty_array@1
0007 splatarray                             true
0009 putobject                              :a
0011 putobject                              2
0013 newhash                                2
0015 opt_send_without_block                 <calldata!mid:keyword, argc:2, ARGS_SPLAT|ARGS_SPLAT_MUT|FCALL|KW_SPLAT>
0017 leave                                                            (   8)[Re]
```

Instructions after:

```
== disasm: #<ISeq:[email protected]:4 (4,0)-(8,3)>
local table (size: 2, argc: 1 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 2] empty_hash@0<Arg>[ 1] empty_array@1
0000 newarray                               0                         (   5)[LiCa]
0002 setlocal_WC_0                          empty_array@1
0004 putself                                                          (   7)[Li]
0005 getlocal_WC_0                          empty_array@1
0007 splatarray                             false
0009 putobject                              {:a=>2}
0011 opt_send_without_block                 <calldata!mid:keyword, argc:2, ARGS_SPLAT|FCALL|KW_SPLAT>
0013 leave                                                            (   8)[Re]
```

Differences:

* `splatarray` is `false` not `true
* `putobject`, `putobject`, `newhash` is simply `putobject` with
optimizations on
* Remove `ARGS_SPLAT_MUT` flag

Related: ruby/prism#2994

Co-authored-by: Kevin Newton <[email protected]>
@kddnewton kddnewton assigned XrXr and unassigned eileencodes Aug 28, 2024
This was referenced Aug 28, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@XrXr XrXr

Labels
bug Something isn't working compiler
Projects
None yet
Milestone
CRuby unblocked
Development

Successfully merging a pull request may close this issue.

[PRISM] Set use_block parameter flag for forwardable methods XrXr/ruby
3 participants
@eileencodes @kddnewton @XrXr