1552 Commits

Author SHA1 Message Date
Nobuyoshi Nakada
2e3f81838c
Align styles [ci skip] 2025-05-15 17:48:40 +09:00
Jean Boussier
3ec7bfff2e Use a set_table for rb_vm_struct.unused_block_warning_table
Now that we have a hash-set implementation we can use that
instead of a hash-table with a static value.
2025-04-27 11:59:28 +02:00
Aaron Patterson
f15d85f086 Add parse.y implementation 2025-04-25 13:46:05 -07:00
Yusuke Endoh
0d6263bd41 Fix coverage measurement for negative line numbers
Fixes [Bug #21220]

Co-Authored-By: Mike Bourgeous <mike@mikebourgeous.com>
Co-Authored-By: Jean Boussier <jean.boussier@gmail.com>
2025-04-09 23:45:54 +09:00
Jean Boussier
4aa74b605c compile.c: avoid allocating 0 length call_data
if `body->ci_size` is `0`, there's no point allocating 0B,
it just wastes an entry in the allocator.
2025-04-03 16:57:57 +02:00
Aaron Patterson
62cc3464d9 Remove leading nop from block when we don't need it
Blocks insert a leading `nop` instruction in order to execute a "block
call" tracepoint. Block compilation unconditionally inserts a leading
`nop` plus a label after the instruction:

  641f15b1c6/prism_compile.c (L6867-L6869)

This `nop` instruction is used entirely for firing the block entry
tracepoint.  The label exists so that the block can contain a loop but
the block entry tracepoint is executed only once.

For example, the following code is an infinite loop, but should only
execute the b_call tracepoint once:

```ruby
-> { redo }.call
```

Previous to this commit, we would eliminate the `nop` instruction, but
only if there were no other jump instructions inside the block.  This
means that the following code would still contain a leading `nop` even
though the label following the `nop` is unused:

```ruby
-> { nil if bar }
```

```
== disasm: #<ISeq:block in <main>@test.rb:1 (1,2)-(1,17)> (catch: FALSE)
0000 nop                                                              (   1)[Bc]
0001 putself                                [Li]
0002 opt_send_without_block                 <calldata!mid:bar, argc:0, FCALL|VCALL|ARGS_SIMPLE>
0004 branchunless                           8
0006 putnil
0007 leave                                  [Br]
0008 putnil
0009 leave                                  [Br]
```

This commit checks to see if the label inserted after the `nop` is
actually a jump target.  If it's not a jump target, then we should be
safe to eliminate the leading `nop`:

```
> build-master/miniruby --dump=insns test.rb
== disasm: #<ISeq:<main>@test.rb:1 (1,0)-(1,17)>
0000 putspecialobject                       1                         (   1)[Li]
0002 send                                   <calldata!mid:lambda, argc:0, FCALL>, block in <main>
0005 leave

== disasm: #<ISeq:block in <main>@test.rb:1 (1,2)-(1,17)>
0000 putself                                                          (   1)[LiBc]
0001 opt_send_without_block                 <calldata!mid:bar, argc:0, FCALL|VCALL|ARGS_SIMPLE>
0003 branchunless                           7
0005 putnil
0006 leave                                  [Br]
0007 putnil
0008 leave                                  [Br]
```

We have a test for b_call tracepoints that use `redo` here:

  aebf96f371/test/ruby/test_settracefunc.rb (L1728-L1736)
2025-03-20 11:49:13 -07:00
Eileen M. Uchitelle
c85dffeee2
Avoid pinning storage_head in iseq_mark_and_move (#12880)
* Avoid pinning `storage_head` in `iseq_mark_and_move`

This refactor changes the behavior of `iseq_mark_and_move` to avoid
pinning the `storage_head`. Previously pinning was required because they
could be gc'd during `iseq_set_sequence` it would be possible to end
up with a half build array of instructions. However, in order to
implement a moving immix algorithm we can't pin these objects so this
rafactoring changes the code to mark and move. To accomplish this, it was
required to add `iseq_size`, `iseq_encoded`, and the `mark_bits` union
to the `iseq_compile_data` struct. In addition `iseq_compile_data` sets
a bool for whether there is a single or list of mark bits. While this
change is needed for moving immix, it should be better for Ruby's GC
as well.

* Don't allocate mark_offset_bits for one word

If only one word is needed, we don't need to allocate mark_offset_bits
and can instead directly write to it.

---------

Co-authored-by: Peter Zhu <peter@peterzhu.ca>
2025-03-17 10:42:48 -04:00
Jeremy Evans
e0d600ec19
Avoid opt_aset_with optimization inside multiple assignment
Previously, since the opt_aset_with optimization was introduced,
use of the opt_aset_with optimization inside multiple assignment
would result in a segfault or incorrect instructions.

Fixes [Bug #21012]

Co-authored-by: Nobuyoshi Nakada <nobu.nakada@gmail.com>
2025-01-08 08:49:51 -08:00
Nobuyoshi Nakada
b81db531b2
[Bug #21011] nd_value is NULL in massign 2025-01-08 13:26:14 +09:00
ydah
76759003f5 Introduce macro for creating child iseqs with callbacks
Use macro like `NEW_ISEQ` and `NEW_CHILD_ISEQ`.
2025-01-08 10:46:23 +09:00
ydah
b3de2c334e Use ISEQ_BODY(iseq) instead of iseq->body
trivial change that unifies the format for access.
2025-01-07 07:18:49 +09:00
ydah
7124c34f51 Remove unused FIXNUM_OR macro from compile.c
This PR remove unused FIXNUM_OR macro from compile.c

before:
```
❯ git grep 'FIXNUM_OR'
compile.c:#define FIXNUM_OR(n, i) ((n)|INT2FIX(i))
```

after:
```
❯ git grep 'FIXNUM_OR'

```
2025-01-04 18:04:42 +09:00
Nobuyoshi Nakada
6bbb470dc7
[Bug #20504] Move dynamic regexp concatenation to iseq compiler 2025-01-03 10:25:15 +09:00
tompng
e06b3b5ad1 [Bug #20927] Fix compile_shareable_literal_constant for hash with keyword splat
Compilation of NODE_HASH in compile_shareable_literal_constant does not support hash that contains keyword splat.
If there is a keyword splat, fallback to default case.
2024-12-15 16:55:30 +09:00
ydah
6ccaa37eb3 [Bug #20926] Fix a crashes with shareable_constant_value: experimental_everything using parse.y's parser
https://bugs.ruby-lang.org/issues/20926
2024-12-06 12:32:20 +09:00
Yusuke Endoh
7f34c75b8b Use RSTRING_PTR instead of StringValuePtr
... since it is certain to be a String in this context.

Also, I want to avoid the anxious use of `StringValuePtr(str)` and
`RSTRING_LEN(str)` as arguments in the same function call.
2024-11-30 03:15:03 +09:00
Nobuyoshi Nakada
de89bff122
INIT_ANCHOR no longer needed usually 2024-11-28 19:02:56 +09:00
Nobuyoshi Nakada
b8f248ee5a
Initialize LINK_ANCHOR totally 2024-11-28 18:53:10 +09:00
Nobuyoshi Nakada
319ac31529
Assert that non-empty LINK_ANCHOR does not loop
After any `LINK_ELEMENT` sequence is added, `LINK_ANCHOR` must not
loop.
2024-11-28 15:13:52 +09:00
Randy Stauner
1dd40ec18a
Optimize instructions when creating an array just to call include? (#12123)
* Add opt_duparray_send insn to skip the allocation on `#include?`

If the method isn't going to modify the array we don't need to copy it.
This avoids the allocation / array copy for things like `[:a, :b].include?(x)`.

This adds a BOP for include? and tracks redefinition for it on Array.

Co-authored-by: Andrew Novoselac <andrew.novoselac@shopify.com>

* YJIT: Implement opt_duparray_send include_p

Co-authored-by: Andrew Novoselac <andrew.novoselac@shopify.com>

* Update opt_newarray_send to support simple forms of include?(arg)

Similar to opt_duparray_send but for non-static arrays.

* YJIT: Implement opt_newarray_send include_p

---------

Co-authored-by: Andrew Novoselac <andrew.novoselac@shopify.com>
2024-11-26 14:31:08 -05:00
Jean byroot Boussier
6deeec5d45
Mark strings returned by Symbol#to_s as chilled (#12065)
* Use FL_USER0 for ELTS_SHARED

This makes space in RString for two bits for chilled strings.

* Mark strings returned by `Symbol#to_s` as chilled

[Feature #20350]

`STR_CHILLED` now spans on two user flags. If one bit is set it
marks a chilled string literal, if it's the other it marks a
`Symbol#to_s` chilled string.

Since it's not possible, and doesn't make much sense to include
debug info when `--debug-frozen-string-literal` is set, we can't
include allocation source, but we can safely include the symbol
name in the warning message, making it much easier to find the source
of the issue.

Co-Authored-By: Étienne Barrié <etienne.barrie@gmail.com>

---------

Co-authored-by: Étienne Barrié <etienne.barrie@gmail.com>
Co-authored-by: Jean Boussier <jean.boussier@gmail.com>
2024-11-13 09:20:00 -05:00
Koichi Sasada
ab7ab9e450 Warning[:strict_unused_block]
to show unused block warning strictly.

```ruby
class C
  def f = nil
end

class D
  def f = yield
end

[C.new, D.new].each{|obj| obj.f{}}
```

In this case, `D#f` accepts a block. However `C#f` doesn't
accept a block. There are some cases passing a block with
`obj.f{}` where `obj` is `C` or `D`. To avoid warnings on
such cases, "unused block warning" will be warned only if
there is not same name which accepts a block.
On the above example, `C.new.f{}` doesn't show any warnings
because there is a same name `D#f` which accepts a block.

We call this default behavior as "relax mode".

`strict_unused_block` new warning category changes from
"relax mode" to "strict mode", we don't check same name
methods and `C.new.f{}` will be warned.

[Feature #15554]
2024-11-06 11:06:18 +09:00
Takashi Kokubun
478e0fc710
YJIT: Replace Array#each only when YJIT is enabled (#11955)
* YJIT: Replace Array#each only when YJIT is enabled

* Add comments about BUILTIN_ATTR_C_TRACE

* Make Ruby Array#each available with --yjit as well

* Fix all paths that expect a C location

* Use method_basic_definition_p to detect patches

* Copy a comment about C_TRACE flag to compilers

* Rephrase a comment about add_yjit_hook

* Give METHOD_ENTRY_BASIC flag to Array#each

* Add --yjit-c-builtin option

* Allow inconsistent source_location in test-spec

* Refactor a check of BUILTIN_ATTR_C_TRACE

* Set METHOD_ENTRY_BASIC without touching vm->running
2024-11-04 11:14:28 -05:00
Étienne Barrié
257f78fb67 Show where mutated chilled strings were allocated
[Feature #20205]

The warning now suggests running with --debug-frozen-string-literal:

```
test.rb:3: warning: literal string will be frozen in the future (run with --debug-frozen-string-literal for more information)
```

When using --debug-frozen-string-literal, the location where the string
was created is shown:

```
test.rb:3: warning: literal string will be frozen in the future
test.rb:1: info: the string was created here
```

When resurrecting strings and debug mode is not enabled, the overhead is a simple FL_TEST_RAW.
When mutating chilled strings and deprecation warnings are not enabled,
the overhead is a simple warning category enabled check.

Co-authored-by: Jean Boussier <byroot@ruby-lang.org>
Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org>
Co-authored-by: Jean Boussier <byroot@ruby-lang.org>
2024-10-21 12:33:02 +02:00
Kevin Newton
e17243d325 Point keyword->table into iseq local table 2024-10-18 14:16:02 -04:00
Nobuyoshi Nakada
9a90cd2284 Cast via uintptr_t function pointer between object pointer
- ISO C forbids conversion of function pointer to object pointer type
- ISO C forbids conversion of object pointer to function pointer type
2024-10-08 23:29:49 +09:00
Luke Gruber
d592ddd5e6 Fix compile issue with a short-circuited if/unless condition and defined?
This caused an issue when `defined?` was in the `if` condition. Its
instructions weren't appended to the instruction sequence even though it was compiled
if a compile-time known logical short-circuit happened before the `defined?`. The catch table
entry (`defined?` compilation produces a catch table entry) was still on the iseq even though the
instructions weren't there. This caused faulty exception handling in the method.
The solution is to no add the catch table entry for `defined?` after a compile-time known logical
short circuit.

This shouldn't touch much code, it's only for cases like the following,
which can occur during debugging:

    if false && defined?(Some::CONSTANT)
    "more code..."
    end

Fixes [Bug #20501]
2024-10-01 02:12:56 +09:00
Yudai Takada
0209b9554d
Extract setup_branch
From duplicate code in `decl_branch_base` and `add_trace_branch_coverage`.
2024-09-27 01:03:40 +09:00
Peter Zhu
e02a6097e6 Set node_id to -1 in add_adjust_info
add_adjust_info will increment the insns_info_index, so we need to set
the node_id to -1 to prevent a "Conditional jump or move depends on
uninitialised value" in Valgrind.
2024-09-24 08:59:15 -04:00
Peter Zhu
2230ac4a28 Fix potentially missing write barrier in iseq_build_kw
We're writing objects to the iseq but not firing the write barrier.
2024-09-19 14:51:21 -04:00
Peter Zhu
ead280fe8b Replace RB_OBJ_WRITTEN with RB_OBJ_WRITE in iseq_set_arguments_keywords 2024-09-19 14:51:21 -04:00
Jeremy Evans
29f2cb83fb
Fix evaluation order issue in f(**h, &h.delete(key))
Previously, this would delete the key in `h` before keyword
splatting `h`.  This goes against how ruby handles `f(*a, &a.pop)`
and similar expressions.

Fix this by having the compiler check whether the block pass
expression is safe.  If it is not safe, then dup the keyword
splatted hash before evaluating the block pass expression.

For expression: `h=nil; f(**h, &h.delete(:key))`

VM instructions before:

```
0000 putnil                                                           (   1)[Li]
0001 setlocal_WC_0                          h@0
0003 putself
0004 getlocal_WC_0                          h@0
0006 getlocal_WC_0                          h@0
0008 putobject                              :key
0010 opt_send_without_block                 <calldata!mid:delete, argc:1, ARGS_SIMPLE>
0012 splatkw
0013 send                                   <calldata!mid:f, argc:1, ARGS_BLOCKARG|FCALL|KW_SPLAT>, nil
0016 leave
```

VM instructions after:

```
0000 putnil                                                           (   1)[Li]
0001 setlocal_WC_0                          h@0
0003 putself
0004 putspecialobject                       1
0006 newhash                                0
0008 getlocal_WC_0                          h@0
0010 opt_send_without_block                 <calldata!mid:core#hash_merge_kwd, argc:2, ARGS_SIMPLE>
0012 getlocal_WC_0                          h@0
0014 putobject                              :key
0016 opt_send_without_block                 <calldata!mid:delete, argc:1, ARGS_SIMPLE>
0018 send                                   <calldata!mid:f, argc:1, ARGS_BLOCKARG|FCALL|KW_SPLAT|KW_SPLAT_MUT>, nil
0021 leave
```

This is the same as 07d3bf4832532ae7446c9a6924d79aed60a7a9a5, except that
it removes unnecessary hash allocations when using the prism compiler.

Fixes [Bug #20640]
2024-09-18 12:46:07 -07:00
Jeremy Evans
9c12c39ed1 Revert "Fix evaluation order issue in f(**h, &h.delete(key))"
This reverts commit 07d3bf4832532ae7446c9a6924d79aed60a7a9a5.

No failures in the pull request CI, but there are now allocation
test failures.
2024-09-18 11:26:10 -07:00
Jeremy Evans
07d3bf4832
Fix evaluation order issue in f(**h, &h.delete(key))
Previously, this would delete the key in h before keyword
splatting h.  This goes against how ruby handles f(*a, &a.pop)
and similar expressions.

Fix this by having the compiler check whether the block pass
expression is safe.  If it is not safe, then dup the keyword
splatted hash before evaluating the block pass expression.

For expression: `h=nil; f(**h, &h.delete(:key))`

VM instructions before:

```
0000 putnil                                                           (   1)[Li]
0001 setlocal_WC_0                          h@0
0003 putself
0004 getlocal_WC_0                          h@0
0006 getlocal_WC_0                          h@0
0008 putobject                              :key
0010 opt_send_without_block                 <calldata!mid:delete, argc:1, ARGS_SIMPLE>
0012 splatkw
0013 send                                   <calldata!mid:f, argc:1, ARGS_BLOCKARG|FCALL|KW_SPLAT>, nil
0016 leave
```

VM instructions after:

```
0000 putnil                                                           (   1)[Li]
0001 setlocal_WC_0                          h@0
0003 putself
0004 putspecialobject                       1
0006 newhash                                0
0008 getlocal_WC_0                          h@0
0010 opt_send_without_block                 <calldata!mid:core#hash_merge_kwd, argc:2, ARGS_SIMPLE>
0012 getlocal_WC_0                          h@0
0014 putobject                              :key
0016 opt_send_without_block                 <calldata!mid:delete, argc:1, ARGS_SIMPLE>
0018 send                                   <calldata!mid:f, argc:1, ARGS_BLOCKARG|FCALL|KW_SPLAT|KW_SPLAT_MUT>, nil
0021 leave
```

Fixes [Bug #20640]
2024-09-18 11:18:29 -07:00
Étienne Barrié
bf9879791a Optimized instruction for Hash#freeze
If a Hash which is empty or only using literals is frozen, we detect
this as a peephole optimization and change the instructions to be
`opt_hash_freeze`.

[Feature #20684]

Co-authored-by: Jean Boussier <byroot@ruby-lang.org>
2024-09-05 12:46:02 +02:00
Étienne Barrié
a99707cd9c Optimized instruction for Array#freeze
If an Array which is empty or only using literals is frozen, we detect
this as a peephole optimization and change the instructions to be
`opt_ary_freeze`.

[Feature #20684]

Co-authored-by: Jean Boussier <byroot@ruby-lang.org>
2024-09-05 12:46:02 +02:00
Jeremy Evans
a3562c2a0a
Remove incorrect setting of KW_SPLAT_MUT flag
Fixes [Bug #20701]

Co-authored-by: Pablo Herrero <pablodherrero@gmail.com>
2024-08-27 09:08:22 -07:00
Nobuyoshi Nakada
f73d435262
Check compile_branch_condition results 2024-08-20 12:26:02 +09:00
tompng
992596fb7a Fix next inside block argument stack underflow
[Bug #20344]
Fix compile_next adding removable adjust label
2024-08-12 18:09:32 +09:00
Jean Boussier
ca5b7276c6 compile.c: don't allocate empty default values list
It just wastes memory.
2024-08-11 15:04:35 +02:00
tomoya ishida
1870505f47
Fix wrong unreachable chunk remove when jump destination label is unremovable 2024-07-30 15:31:58 +09:00
Randy Stauner
acbb8d4fb5 Expand opt_newarray_send to support Array#pack with buffer keyword arg
Use an enum for the method arg instead of needing to add an id
that doesn't map to an actual method name.

$ ruby --dump=insns -e 'b = "x"; [v].pack("E*", buffer: b)'

before:

```
== disasm: #<ISeq:<main>@-e:1 (1,0)-(1,34)>
local table (size: 1, argc: 0 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 1] b@0
0000 putchilledstring                       "x"                       (   1)[Li]
0002 setlocal_WC_0                          b@0
0004 putself
0005 opt_send_without_block                 <calldata!mid:v, argc:0, FCALL|VCALL|ARGS_SIMPLE>
0007 newarray                               1
0009 putchilledstring                       "E*"
0011 getlocal_WC_0                          b@0
0013 opt_send_without_block                 <calldata!mid:pack, argc:2, kw:[#<Symbol:0x000000000023110c>], KWARG>
0015 leave
```

after:

```
== disasm: #<ISeq:<main>@-e:1 (1,0)-(1,34)>
local table (size: 1, argc: 0 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 1] b@0
0000 putchilledstring                       "x"                       (   1)[Li]
0002 setlocal_WC_0                          b@0
0004 putself
0005 opt_send_without_block                 <calldata!mid:v, argc:0, FCALL|VCALL|ARGS_SIMPLE>
0007 putchilledstring                       "E*"
0009 getlocal                               b@0, 0
0012 opt_newarray_send                      3, 5
0015 leave
```
2024-07-29 16:26:58 -04:00
Nobuyoshi Nakada
5f1eb0dc79
Fix wrong conversion in disasm dump
`LINK_ELEMENT::type` is an `enum` not a `VALUE`, `FIX2LONG` doesn't
make sense.
2024-07-26 11:24:53 +09:00
yui-knk
f2728c3393 Change RESBODY Node structure
Extracrt exception variable into `nd_exc_var` field
to keep the original grammar structure.

For example:

```
begin
rescue Error => e1
end
```

Before:

```
@ NODE_RESBODY (id: 8, line: 2, location: (2,0)-(2,18))
+- nd_args:
|   @ NODE_LIST (id: 2, line: 2, location: (2,7)-(2,12))
|   +- as.nd_alen: 1
|   +- nd_head:
|   |   @ NODE_CONST (id: 1, line: 2, location: (2,7)-(2,12))
|   |   +- nd_vid: :Error
|   +- nd_next:
|       (null node)
+- nd_body:
|   @ NODE_BLOCK (id: 6, line: 2, location: (2,13)-(2,18))
|   +- nd_head (1):
|   |   @ NODE_LASGN (id: 3, line: 2, location: (2,13)-(2,18))
|   |   +- nd_vid: :e1
|   |   +- nd_value:
|   |       @ NODE_ERRINFO (id: 5, line: 2, location: (2,13)-(2,18))
|   +- nd_head (2):
|       @ NODE_BEGIN (id: 4, line: 2, location: (2,18)-(2,18))
|       +- nd_body:
|           (null node)
+- nd_next:
    (null node)
```

After:

```
@ NODE_RESBODY (id: 6, line: 2, location: (2,0)-(2,18))
+- nd_args:
|   @ NODE_LIST (id: 2, line: 2, location: (2,7)-(2,12))
|   +- as.nd_alen: 1
|   +- nd_head:
|   |   @ NODE_CONST (id: 1, line: 2, location: (2,7)-(2,12))
|   |   +- nd_vid: :Error
|   +- nd_next:
|       (null node)
+- nd_exc_var:
|   @ NODE_LASGN (id: 3, line: 2, location: (2,13)-(2,18))
|   +- nd_vid: :e1
|   +- nd_value:
|       @ NODE_ERRINFO (id: 5, line: 2, location: (2,13)-(2,18))
+- nd_body:
|   @ NODE_BEGIN (id: 4, line: 2, location: (2,18)-(2,18))
|   +- nd_body:
|       (null node)
+- nd_next:
    (null node)
```
2024-07-26 07:29:32 +09:00
yui-knk
6be539aab5 Change UNDEF Node structure
Change UNDEF Node to hold their items to keep the original grammar
structure.

For example:

```
undef a, b
```

Before:

```
@ NODE_BLOCK (id: 4, line: 1, location: (1,6)-(1,10))*
+- nd_head (1):
|   @ NODE_UNDEF (id: 1, line: 1, location: (1,6)-(1,7))
|   +- nd_undef:
|       @ NODE_SYM (id: 0, line: 1, location: (1,6)-(1,7))
|       +- string: :a
+- nd_head (2):
    @ NODE_UNDEF (id: 3, line: 1, location: (1,9)-(1,10))
    +- nd_undef:
        @ NODE_SYM (id: 2, line: 1, location: (1,9)-(1,10))
        +- string: :b
```

After:

```
@ NODE_UNDEF (id: 1, line: 1, location: (1,6)-(1,10))*
+- nd_undefs:
    +- length: 2
    +- element (0):
    |   @ NODE_SYM (id: 0, line: 1, location: (1,6)-(1,7))
    |   +- string: :a
    +- element (1):
        @ NODE_SYM (id: 2, line: 1, location: (1,9)-(1,10))
        +- string: :b
```
2024-07-20 11:25:26 +09:00
Jeremy Evans
2c79a7641f Remove splatarray true -> splatarray false peephole optimization
The compiler now uses splatarray false for all cases that would
previously have been optimized, so this is all dead code.
2024-07-18 22:17:21 -07:00
Jeremy Evans
3de20efc30 Avoid unnecessary array allocations for f(arg, *arg, **arg, **arg), f(*arg, 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.
2024-07-18 22:17:21 -07:00
Nobuyoshi Nakada
60d3ed5043 [Bug #20457] Drop unreachable return at end of method 2024-07-16 22:11:28 +09:00
Jeremy Evans
0ee3960685 Eliminate array allocations for single splat followed by mutable keywords
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.
2024-07-10 14:45:38 -07:00
eileencodes
d25b74b32c Resize arrays in rb_ary_freeze and use it for freezing arrays
While working on a separate issue we found that in some cases
`ary_heap_realloc` was being called on frozen arrays. To fix this, this
change does the following:

1) Updates `rb_ary_freeze` to assert the type is an array, return if
already frozen, and shrink the capacity if it is not embedded, shared
or a shared root.
2) Replaces `rb_obj_freeze` with `rb_ary_freeze` when the object is
always an array.
3) In `ary_heap_realloc`, ensure the new capa is set with
`ARY_SET_CAPA`. Previously the change in capa was not set.
4) Adds an assertion to `ary_heap_realloc` that the array is not frozen.

Some of this work was originally done in
https://github.com/ruby/ruby/pull/2640, referencing this issue
https://bugs.ruby-lang.org/issues/16291. There didn't appear to be any
objections to this PR, it appears to have simply lost traction.

The original PR made changes to arrays and strings at the same time,
this PR only does arrays. Also it was old enough that rather than revive
that branch I've made a new one. I added Lourens as co-author in addtion
to Aaron who helped me with this patch.

The original PR made this change for performance reasons, and while
that's still true for this PR, the goal of this PR is to avoid
calling `ary_heap_realloc` on frozen arrays. The capacity should be
shrunk _before_ the array is frozen, not after.

Co-authored-by: Aaron Patterson <tenderlove@ruby-lang.org>
Co-Authored-By: methodmissing <lourens@methodmissing.com>
2024-07-02 10:34:23 -07:00