Misc #20630
closedPotential optimizations for NODE_CONST compilation
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 -
opt_getconstant_path
peephole optimization: b8ece8c
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:
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 (John Hawthorn) ?)
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
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
Updated by zack.ref@gmail.com (Zack Deveau) 5 months ago
- Subject changed from Potential optimizations for `NODE_CONST` compilation to Potential optimizations for NODE_CONST compilation
Updated by zack.ref@gmail.com (Zack Deveau) 5 months ago
- Description updated (diff)
Updated by ufuk (Ufuk Kayserilioglu) 5 months ago
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:
# 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.
Updated by zack.ref@gmail.com (Zack Deveau) 5 months ago
- Assignee set to zack.ref@gmail.com (Zack Deveau)
Updated by zack.ref@gmail.com (Zack Deveau) 5 months ago
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!
Updated by jeremyevans0 (Jeremy Evans) 5 months ago
- Status changed from Open to Rejected