[ruby-core:118569] [Ruby master Misc#20630] Potential optimizations for `NODE_CONST` compilation

[ruby-core:118569] [Ruby master Misc#20630] Potential optimizations for `NODE_CONST` compilation

[zack.ref@gmail.com (Zack Deveau) via ruby-core]

Issue #20630 has been reported by zack.ref@gmail.com (Zack Deveau).

----------------------------------------
Misc #20630: Potential optimizations for `NODE_CONST` compilation
https://bugs.ruby-lang.org/issues/20630

* Author: zack.ref@gmail.com (Zack Deveau)
* Status: Open
----------------------------------------
### Description

I would like to propose two potential optimizations to the way we currently compile `NODE_CONST` nodes in `compile.c`. I've included commits for each on a related PR.

- PR: https://github.com/ruby/ruby/pull/11154
- `NODE_CONST` compilation: [9b6d613](https://github.com/ruby/ruby/commit/9b6d613e19c1132ace64583ac23c94fcbc77f1df)
- `opt_getconstant_path` peephole optimization: [b8ece8c](https://github.com/ruby/ruby/commit/b8ece8ca6e2d60cb7c627927aa89c1e8980ad0eb)

These commits pass `test-all` locally.

### `NODE_CONST` Compilation

From what I can tell `NODE_CONST` doesn't appear to follow the conventional use of `popped` found in other similar nodes. For example `NODE_IVAR`, when `popped` (return value not used), will avoid adding the YARV instruction altogether:
```C
      case NODE_IVAR:{
        debugi("nd_vid", RNODE_IVAR(node)->nd_vid);
        if (!popped) {
            ADD_INSN2(ret, node, getinstancevariable,
                      ID2SYM(RNODE_IVAR(node)->nd_vid),
                      get_ivar_ic_value(iseq, RNODE_IVAR(node)->nd_vid));
        }
        break;
      }

```

When compiling a `NODE_CONST` that is `popped`, we instead add either `get_constant` or the optimized `opt_getconstant_path` instruction, followed by adding a `pop`.

I've modified it to follow the convention found elsewhere to avoid adding either instruction(s) in cases where we don't need the return value. `test-all` passes locally and I can't think of meaningful side effects (other than we avoid exercising the cache). Please let me know if I'm missing any!

### `opt_getconstant_path` peephole optimization

`iseq_peephole_optimize` includes an optimization for `insn -> pop` sequences. Most of the `get_x` instructions are included but we don't appear to include `opt_getconstant_path`. (potentially since it was only recently added in 2022 by @jhawthorn ?)

I've included it and attempted to account for any meaningful side effects (here again I can't think of any other than we avoid exercising the cache). Please let me know if I missed any.


### Results

`test.rb`
```Ruby
NETSCAPE = "navigator"
[NETSCAPE, NETSCAPE, NETSCAPE, NETSCAPE]
1
```

`ruby 3.4.0dev (2024-07-11T19:49:14Z master 6fc83118bb) [arm64-darwin23]`
```
💾 ➜   ruby --dump insn ./test.rb
== disasm: #<ISeq:<main>@./test.rb:1 (1,0)-(3,1)>
0000 putchilledstring                       "navigator"               (   1)[Li]
0002 putspecialobject                       3
0004 setconstant                            :NETSCAPE
0006 opt_getconstant_path                   <ic:0 NETSCAPE>           (   2)[Li]
0008 pop
0009 opt_getconstant_path                   <ic:1 NETSCAPE>
0011 pop
0012 opt_getconstant_path                   <ic:2 NETSCAPE>
0014 pop
0015 opt_getconstant_path                   <ic:3 NETSCAPE>
0017 pop
0018 putobject_INT2FIX_1_                                             (   3)[Li]
0019 leave
```


`with optimizations`
```
💾 ➜   ./build/miniruby --dump insn ./test.rb
== disasm: #<ISeq:<main>@./test.rb:1 (1,0)-(3,1)>
0000 putchilledstring                       "navigator"               (   1)[Li]
0002 putspecialobject                       3
0004 setconstant                            :NETSCAPE
0006 putobject_INT2FIX_1_                                             (   3)[Li]
0007 leave
```




-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:118570] [Ruby master Misc#20630] Potential optimizations for NODE_CONST compilation

[ufuk (Ufuk Kayserilioglu) via ruby-core]

Issue #20630 has been updated by ufuk (Ufuk Kayserilioglu).


I am not an expert, but I don't think you can optimize away constant references just because their value is not being used. Constant references might have side effects, in particular autoloading, that will change behaviour if the reference is removed by the compiler.

For example, the following behaviour should hold:
```ruby
# a.rb
class A
end

class Z
end


# b.rb
autoload :A, "./a.rb"
A
p defined?(Z) # => :constant
```

In your "optimization", I expect this would print `nil` thus changing behaviour.

----------------------------------------
Misc #20630: Potential optimizations for NODE_CONST compilation
https://bugs.ruby-lang.org/issues/20630#change-109094

* Author: zack.ref@gmail.com (Zack Deveau)
* Status: Open
----------------------------------------
I would like to propose two potential optimizations to the way we currently compile `NODE_CONST` nodes in `compile.c`. I've included commits for each on a related PR.

- PR: https://github.com/ruby/ruby/pull/11154
- `NODE_CONST` compilation: [9b6d613](https://github.com/ruby/ruby/commit/9b6d613e19c1132ace64583ac23c94fcbc77f1df)
- `opt_getconstant_path` peephole optimization: [b8ece8c](https://github.com/ruby/ruby/commit/b8ece8ca6e2d60cb7c627927aa89c1e8980ad0eb)

These commits pass `test-all` locally.

### `NODE_CONST` Compilation

From what I can tell `NODE_CONST` doesn't appear to follow the conventional use of `popped` found in other similar nodes. For example `NODE_IVAR`, when `popped` (return value not used), will avoid adding the YARV instruction altogether:
```C
      case NODE_IVAR:{
        debugi("nd_vid", RNODE_IVAR(node)->nd_vid);
        if (!popped) {
            ADD_INSN2(ret, node, getinstancevariable,
                      ID2SYM(RNODE_IVAR(node)->nd_vid),
                      get_ivar_ic_value(iseq, RNODE_IVAR(node)->nd_vid));
        }
        break;
      }

```

When compiling a `NODE_CONST` that is `popped`, we instead add either `get_constant` or the optimized `opt_getconstant_path` instruction, followed by adding a `pop`.

I've modified it to follow the convention found elsewhere to avoid adding either instruction(s) in cases where we don't need the return value. `test-all` passes locally and I can't think of meaningful side effects (other than we avoid exercising the cache). Please let me know if I'm missing any!

### `opt_getconstant_path` peephole optimization

`iseq_peephole_optimize` includes an optimization for `insn -> pop` sequences. Most of the `get_x` instructions are included but we don't appear to include `opt_getconstant_path`. (potentially since it was only recently added in 2022 by @jhawthorn ?)

I've included it and attempted to account for any meaningful side effects (here again I can't think of any other than we avoid exercising the cache). Please let me know if I missed any.


### Results

`test.rb`
```Ruby
NETSCAPE = "navigator"
[NETSCAPE, NETSCAPE, NETSCAPE, NETSCAPE]
1
```

`ruby 3.4.0dev (2024-07-11T19:49:14Z master 6fc83118bb) [arm64-darwin23]`
```
💾 ➜   ruby --dump insn ./test.rb
== disasm: #<ISeq:<main>@./test.rb:1 (1,0)-(3,1)>
0000 putchilledstring                       "navigator"               (   1)[Li]
0002 putspecialobject                       3
0004 setconstant                            :NETSCAPE
0006 opt_getconstant_path                   <ic:0 NETSCAPE>           (   2)[Li]
0008 pop
0009 opt_getconstant_path                   <ic:1 NETSCAPE>
0011 pop
0012 opt_getconstant_path                   <ic:2 NETSCAPE>
0014 pop
0015 opt_getconstant_path                   <ic:3 NETSCAPE>
0017 pop
0018 putobject_INT2FIX_1_                                             (   3)[Li]
0019 leave
```


`with optimizations`
```
💾 ➜   ./build/miniruby --dump insn ./test.rb
== disasm: #<ISeq:<main>@./test.rb:1 (1,0)-(3,1)>
0000 putchilledstring                       "navigator"               (   1)[Li]
0002 putspecialobject                       3
0004 setconstant                            :NETSCAPE
0006 putobject_INT2FIX_1_                                             (   3)[Li]
0007 leave
```




-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:118571] [Ruby master Misc#20630] Potential optimizations for NODE_CONST compilation

[zack.ref@gmail.com (Zack Deveau) via ruby-core]

Issue #20630 has been updated by zack.ref@gmail.com (Zack Deveau).

Assignee set to zack.ref@gmail.com (Zack Deveau)

----------------------------------------
Misc #20630: Potential optimizations for NODE_CONST compilation
https://bugs.ruby-lang.org/issues/20630#change-109095

* Author: zack.ref@gmail.com (Zack Deveau)
* Status: Open
* Assignee: zack.ref@gmail.com (Zack Deveau)
----------------------------------------
I would like to propose two potential optimizations to the way we currently compile `NODE_CONST` nodes in `compile.c`. I've included commits for each on a related PR.

- PR: https://github.com/ruby/ruby/pull/11154
- `NODE_CONST` compilation: [9b6d613](https://github.com/ruby/ruby/commit/9b6d613e19c1132ace64583ac23c94fcbc77f1df)
- `opt_getconstant_path` peephole optimization: [b8ece8c](https://github.com/ruby/ruby/commit/b8ece8ca6e2d60cb7c627927aa89c1e8980ad0eb)

These commits pass `test-all` locally.

### `NODE_CONST` Compilation

From what I can tell `NODE_CONST` doesn't appear to follow the conventional use of `popped` found in other similar nodes. For example `NODE_IVAR`, when `popped` (return value not used), will avoid adding the YARV instruction altogether:
```C
      case NODE_IVAR:{
        debugi("nd_vid", RNODE_IVAR(node)->nd_vid);
        if (!popped) {
            ADD_INSN2(ret, node, getinstancevariable,
                      ID2SYM(RNODE_IVAR(node)->nd_vid),
                      get_ivar_ic_value(iseq, RNODE_IVAR(node)->nd_vid));
        }
        break;
      }

```

When compiling a `NODE_CONST` that is `popped`, we instead add either `get_constant` or the optimized `opt_getconstant_path` instruction, followed by adding a `pop`.

I've modified it to follow the convention found elsewhere to avoid adding either instruction(s) in cases where we don't need the return value. `test-all` passes locally and I can't think of meaningful side effects (other than we avoid exercising the cache). Please let me know if I'm missing any!

### `opt_getconstant_path` peephole optimization

`iseq_peephole_optimize` includes an optimization for `insn -> pop` sequences. Most of the `get_x` instructions are included but we don't appear to include `opt_getconstant_path`. (potentially since it was only recently added in 2022 by @jhawthorn ?)

I've included it and attempted to account for any meaningful side effects (here again I can't think of any other than we avoid exercising the cache). Please let me know if I missed any.


### Results

`test.rb`
```Ruby
NETSCAPE = "navigator"
[NETSCAPE, NETSCAPE, NETSCAPE, NETSCAPE]
1
```

`ruby 3.4.0dev (2024-07-11T19:49:14Z master 6fc83118bb) [arm64-darwin23]`
```
💾 ➜   ruby --dump insn ./test.rb
== disasm: #<ISeq:<main>@./test.rb:1 (1,0)-(3,1)>
0000 putchilledstring                       "navigator"               (   1)[Li]
0002 putspecialobject                       3
0004 setconstant                            :NETSCAPE
0006 opt_getconstant_path                   <ic:0 NETSCAPE>           (   2)[Li]
0008 pop
0009 opt_getconstant_path                   <ic:1 NETSCAPE>
0011 pop
0012 opt_getconstant_path                   <ic:2 NETSCAPE>
0014 pop
0015 opt_getconstant_path                   <ic:3 NETSCAPE>
0017 pop
0018 putobject_INT2FIX_1_                                             (   3)[Li]
0019 leave
```


`with optimizations`
```
💾 ➜   ./build/miniruby --dump insn ./test.rb
== disasm: #<ISeq:<main>@./test.rb:1 (1,0)-(3,1)>
0000 putchilledstring                       "navigator"               (   1)[Li]
0002 putspecialobject                       3
0004 setconstant                            :NETSCAPE
0006 putobject_INT2FIX_1_                                             (   3)[Li]
0007 leave
```




-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:118572] [Ruby master Misc#20630] Potential optimizations for NODE_CONST compilation

[zack.ref@gmail.com (Zack Deveau) via ruby-core]

Issue #20630 has been updated by zack.ref@gmail.com (Zack Deveau).


Ah, I figured I was missing something. Thanks for pointing that out.

We can close this in that case. At least it will serve as a note for anyone else who stumbles into this!

----------------------------------------
Misc #20630: Potential optimizations for NODE_CONST compilation
https://bugs.ruby-lang.org/issues/20630#change-109096

* Author: zack.ref@gmail.com (Zack Deveau)
* Status: Open
* Assignee: zack.ref@gmail.com (Zack Deveau)
----------------------------------------
I would like to propose two potential optimizations to the way we currently compile `NODE_CONST` nodes in `compile.c`. I've included commits for each on a related PR.

- PR: https://github.com/ruby/ruby/pull/11154
- `NODE_CONST` compilation: [9b6d613](https://github.com/ruby/ruby/commit/9b6d613e19c1132ace64583ac23c94fcbc77f1df)
- `opt_getconstant_path` peephole optimization: [b8ece8c](https://github.com/ruby/ruby/commit/b8ece8ca6e2d60cb7c627927aa89c1e8980ad0eb)

These commits pass `test-all` locally.

### `NODE_CONST` Compilation

From what I can tell `NODE_CONST` doesn't appear to follow the conventional use of `popped` found in other similar nodes. For example `NODE_IVAR`, when `popped` (return value not used), will avoid adding the YARV instruction altogether:
```C
      case NODE_IVAR:{
        debugi("nd_vid", RNODE_IVAR(node)->nd_vid);
        if (!popped) {
            ADD_INSN2(ret, node, getinstancevariable,
                      ID2SYM(RNODE_IVAR(node)->nd_vid),
                      get_ivar_ic_value(iseq, RNODE_IVAR(node)->nd_vid));
        }
        break;
      }

```

When compiling a `NODE_CONST` that is `popped`, we instead add either `get_constant` or the optimized `opt_getconstant_path` instruction, followed by adding a `pop`.

I've modified it to follow the convention found elsewhere to avoid adding either instruction(s) in cases where we don't need the return value. `test-all` passes locally and I can't think of meaningful side effects (other than we avoid exercising the cache). Please let me know if I'm missing any!

### `opt_getconstant_path` peephole optimization

`iseq_peephole_optimize` includes an optimization for `insn -> pop` sequences. Most of the `get_x` instructions are included but we don't appear to include `opt_getconstant_path`. (potentially since it was only recently added in 2022 by @jhawthorn ?)

I've included it and attempted to account for any meaningful side effects (here again I can't think of any other than we avoid exercising the cache). Please let me know if I missed any.


### Results

`test.rb`
```Ruby
NETSCAPE = "navigator"
[NETSCAPE, NETSCAPE, NETSCAPE, NETSCAPE]
1
```

`ruby 3.4.0dev (2024-07-11T19:49:14Z master 6fc83118bb) [arm64-darwin23]`
```
💾 ➜   ruby --dump insn ./test.rb
== disasm: #<ISeq:<main>@./test.rb:1 (1,0)-(3,1)>
0000 putchilledstring                       "navigator"               (   1)[Li]
0002 putspecialobject                       3
0004 setconstant                            :NETSCAPE
0006 opt_getconstant_path                   <ic:0 NETSCAPE>           (   2)[Li]
0008 pop
0009 opt_getconstant_path                   <ic:1 NETSCAPE>
0011 pop
0012 opt_getconstant_path                   <ic:2 NETSCAPE>
0014 pop
0015 opt_getconstant_path                   <ic:3 NETSCAPE>
0017 pop
0018 putobject_INT2FIX_1_                                             (   3)[Li]
0019 leave
```


`with optimizations`
```
💾 ➜   ./build/miniruby --dump insn ./test.rb
== disasm: #<ISeq:<main>@./test.rb:1 (1,0)-(3,1)>
0000 putchilledstring                       "navigator"               (   1)[Li]
0002 putspecialobject                       3
0004 setconstant                            :NETSCAPE
0006 putobject_INT2FIX_1_                                             (   3)[Li]
0007 leave
```




-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/