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/