Misc #20630: Potential optimizations for NODE_CONST compilation

• Status: Rejected
• Assignee: [e-mail] (Zack Deveau)

[[e-mail] (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
• 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

[[e-mail] (Zack Deveau)]

• Subject changed from Potential optimizations for `NODE_CONST` compilation to Potential optimizations for NODE_CONST compilation

[[e-mail] (Zack Deveau)]

• Description updated (diff)

[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:

# 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.

[[e-mail] (Zack Deveau)]

• Assignee set to [e-mail] (Zack Deveau)

[[e-mail] (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!

[jeremyevans0 (Jeremy Evans)]

• Status changed from Open to Rejected