From f701de830557dac9f0005c129cab483c282a0124 Mon Sep 17 00:00:00 2001 From: Charles Oliver Nutter Date: Mon, 1 Apr 2024 15:51:25 -0500 Subject: [PATCH] Hook up to object-to-symbol paths to FString This helps cases where an fstring (born frozen and dedup'ed) is used in a symbol API where it must be interned. For example see the benchmark and discussion in #3419. --- bench/bench_instance_variable_get.rb | 66 ++++++++++++++++++++ core/src/main/java/org/jruby/RubySymbol.java | 21 +++++-- 2 files changed, 81 insertions(+), 6 deletions(-) create mode 100644 bench/bench_instance_variable_get.rb diff --git a/bench/bench_instance_variable_get.rb b/bench/bench_instance_variable_get.rb new file mode 100644 index 00000000000..0df2343ab1e --- /dev/null +++ b/bench/bench_instance_variable_get.rb @@ -0,0 +1,66 @@ +require 'benchmark/ips' + +# Modified version of benchmark from https://github.com/jruby/jruby/issues/3419 + +class Test + def intitialize + @ivar = 'test' + end + + def direct + @ivar + end + + IVAR_NAME = "@ivar" + def hoist_ivar_name + instance_variable_get IVAR_NAME + end + + def instance_variable_get_symbol + instance_variable_get :"@ivar" + end + + def instance_variable_get_frozen + instance_variable_get('@ivar'.freeze) + end + + def instance_variable_get_frozen_symbol + instance_variable_get('@ivar'.freeze.to_sym) + end +end + +loop { +Benchmark.ips do |benchmark| + instance = Test.new + benchmark.report("direct") {|i| + while i > 0 + i-=1 + instance.direct + end + } + benchmark.report("hoist_ivar_name") {|i| + while i > 0 + i-=1 + instance.hoist_ivar_name + end + } + benchmark.report("ivar get symbol") {|i| + while i > 0 + i-=1 + instance.instance_variable_get_symbol + end + } + benchmark.report("ivar get frozen") {|i| + while i > 0 + i-=1 + instance.instance_variable_get_frozen + end + } + benchmark.report("ivar get frozen sym") {|i| + while i > 0 + i-=1 + instance.instance_variable_get_frozen_symbol + end + } +end +} \ No newline at end of file diff --git a/core/src/main/java/org/jruby/RubySymbol.java b/core/src/main/java/org/jruby/RubySymbol.java index 2d69584720b..1e125c8926d 100644 --- a/core/src/main/java/org/jruby/RubySymbol.java +++ b/core/src/main/java/org/jruby/RubySymbol.java @@ -227,20 +227,29 @@ public RubySymbol asInstanceVariable() { * @return the symbol table entry. */ public static RubySymbol retrieveIDSymbol(IRubyObject name) { - return name instanceof RubySymbol ? - (RubySymbol) name : newIDSymbol(name.getRuntime(), name.convertToString().getByteList()); + if (name instanceof RubySymbol sym) return sym; + + if (name instanceof RubyString.FString fstring) { + return fstring.intern(); + } + + return newIDSymbol(name.getRuntime(), name.convertToString().getByteList()); } /** - * Retrieve an ID symbol but call the handler before any new symbol is added to the symbol table. - * This can be used for verifying the symbol is valid. + * Retrieve an ID symbol but call the handler to verify the symbol is valid. * * @param name to get symbol table entry for (it may be a symbol already) * @return the symbol table entry. */ public static RubySymbol retrieveIDSymbol(IRubyObject name, ObjBooleanConsumer handler) { - if (name instanceof RubySymbol) { - RubySymbol sym = (RubySymbol) name; + if (name instanceof RubySymbol sym) { + handler.accept(sym, false); + return sym; + } + + if (name instanceof RubyString.FString fstring) { + RubySymbol sym = fstring.intern(); handler.accept(sym, false); return sym; }