[ruby-core:118569] [Ruby master Misc#20630] Potential optimizations for `NODE_CONST` compilation
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/
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/
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/
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/
participants (2)
-
ufuk (Ufuk Kayserilioglu) -
zack.ref＠gmail.com (Zack Deveau)