From ebb10c2e95d0862e4288ea4ba56893ba3b0bdb4d Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Wed, 25 Oct 2023 14:28:44 -0400 Subject: [PATCH 1/5] Less Instructions == Less Interpreter looping Attempt at reducing simple instrs which can be subsumed by their neighboring instrs. --- core/src/main/java/org/jruby/ir/IRBuilder.java | 10 ++++++++-- .../java/org/jruby/ir/runtime/IRRuntimeHelpers.java | 6 ++++++ .../src/main/java/org/jruby/ir/targets/JVMVisitor.java | 2 ++ 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java index 533676d1911..09603660cda 100644 --- a/core/src/main/java/org/jruby/ir/IRBuilder.java +++ b/core/src/main/java/org/jruby/ir/IRBuilder.java @@ -440,8 +440,14 @@ private void emitEnsureBlocks(IRLoop loop) { private void determineIfWeNeedLineNumber(Node node) { int currLineNum = node.getLine(); - if (currLineNum != lastProcessedLineNum && !(node instanceof NilImplicitNode)) { // Do not emit multiple line number instrs for the same line - needsLineNumInfo = node.isNewline() ? LineInfo.Coverage : LineInfo.Backtrace; + if (currLineNum != lastProcessedLineNum && !(node instanceof NilImplicitNode)) { + LineInfo needsCoverage = node.isNewline() ? LineInfo.Coverage : null; + // DefNode will set it's own line number as part of impl but if it is for coverage we emit as instr also. + if (needsCoverage != null && (!(node instanceof DefNode) || coverageMode != 0)) { // Do not emit multiple line number instrs for the same line + needsLineNumInfo = node.isNewline() ? needsCoverage : LineInfo.Backtrace; + } + + // This line is already process either by linenum or by instr which emits its own. lastProcessedLineNum = currLineNum; } } diff --git a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java index 44ca6172a04..1f5ff26ab41 100644 --- a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java +++ b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java @@ -1738,6 +1738,7 @@ public static RubyModule newRubyClassFromIR(Ruby runtime, String id, StaticScope @Interp public static void defInterpretedClassMethod(ThreadContext context, IRScope method, IRubyObject obj) { + context.setLine(method.getLine()); String id = method.getId(); RubyClass rubyClass = checkClassForDef(context, id, obj); @@ -1759,6 +1760,7 @@ public static void defCompiledClassMethod(ThreadContext context, MethodHandle ha StaticScope scope, String encodedArgumentDescriptors, IRubyObject obj, boolean maybeRefined, boolean receivesKeywordArgs, boolean needsToFindImplementer) { + context.setLine(line); RubyClass rubyClass = checkClassForDef(context, id, obj); if (maybeRefined) scope.captureParentRefinements(context); @@ -1780,6 +1782,7 @@ public static void defCompiledClassMethod(ThreadContext context, MethodHandle va String encodedArgumentDescriptors, IRubyObject obj, boolean maybeRefined, boolean receivesKeywordArgs, boolean needsToFindImplementer) { + context.setLine(line); RubyClass rubyClass = checkClassForDef(context, id, obj); if (maybeRefined) scope.captureParentRefinements(context); @@ -1803,6 +1806,7 @@ private static RubyClass checkClassForDef(ThreadContext context, String id, IRub @Interp public static void defInterpretedInstanceMethod(ThreadContext context, IRScope method, DynamicScope currDynScope, IRubyObject self) { + context.setLine(method.getLine()); Ruby runtime = context.runtime; RubySymbol methodName = method.getName(); RubyModule rubyClass = findInstanceMethodContainer(context, currDynScope, self); @@ -1827,6 +1831,7 @@ public static void defCompiledInstanceMethod(ThreadContext context, MethodHandle StaticScope scope, String encodedArgumentDescriptors, DynamicScope currDynScope, IRubyObject self, boolean maybeRefined, boolean receivesKeywordArgs, boolean needsToFindImplementer) { + context.setLine(line); Ruby runtime = context.runtime; RubySymbol methodName = runtime.newSymbol(id); RubyModule clazz = findInstanceMethodContainer(context, currDynScope, self); @@ -1849,6 +1854,7 @@ public static void defCompiledInstanceMethod(ThreadContext context, MethodHandle String encodedArgumentDescriptors, DynamicScope currDynScope, IRubyObject self, boolean maybeRefined, boolean receivesKeywordArgs, boolean needsToFindImplementer) { + context.setLine(line); Ruby runtime = context.runtime; RubySymbol methodName = runtime.newSymbol(id); RubyModule clazz = findInstanceMethodContainer(context, currDynScope, self); diff --git a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java index 1f59fffbc3e..57c9c4a47ce 100644 --- a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java +++ b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java @@ -1391,6 +1391,7 @@ public void DefineClassInstr(DefineClassInstr defineclassinstr) { public void DefineClassMethodInstr(DefineClassMethodInstr defineclassmethodinstr) { IRMethod method = defineclassmethodinstr.getMethod(); + jvmMethod().updateLineNumber(method.getLine()); jvmMethod().loadContext(); JVMVisitorMethodContext context = new JVMVisitorMethodContext(); @@ -1426,6 +1427,7 @@ public void DefineInstanceMethodInstr(DefineInstanceMethodInstr defineinstanceme IRBytecodeAdapter m = jvmMethod(); SkinnyMethodAdapter a = m.adapter; + jvmMethod().updateLineNumber(method.getLine()); m.loadContext(); emitMethod(method, context); From 0a79d3cc5a9cbc14a5c6c20f48eb34b2e665c9b3 Mon Sep 17 00:00:00 2001 From: Charles Oliver Nutter Date: Thu, 9 Nov 2023 22:11:02 -0600 Subject: [PATCH 2/5] Expand WeakMap to allow floats and fixnums They will use value comparisons rather than identity comparisons to mimic CRuby's behavior for immediate values. Fixes #7862 --- .../main/java/org/jruby/RubyObjectSpace.java | 54 ++++++++++++------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/jruby/RubyObjectSpace.java b/core/src/main/java/org/jruby/RubyObjectSpace.java index 506bb8bfbbd..e10650c7ab4 100644 --- a/core/src/main/java/org/jruby/RubyObjectSpace.java +++ b/core/src/main/java/org/jruby/RubyObjectSpace.java @@ -37,6 +37,7 @@ import java.util.Iterator; import java.util.Map; +import java.util.stream.Stream; import org.jruby.anno.JRubyMethod; import org.jruby.anno.JRubyModule; @@ -51,6 +52,7 @@ import org.jruby.util.Inspector; import org.jruby.util.Numeric; import org.jruby.util.collections.WeakValuedIdentityMap; +import org.jruby.util.collections.WeakValuedMap; @JRubyModule(name="ObjectSpace") public class RubyObjectSpace { @@ -231,59 +233,74 @@ public WeakMap(Ruby runtime, RubyClass cls) { @JRubyMethod(name = "[]") public IRubyObject op_aref(ThreadContext context, IRubyObject key) { - IRubyObject value = map.get(key); + Map weakMap = getWeakMapFor(key); + IRubyObject value = weakMap.get(key); if (value != null) return value; return context.nil; } + private Map getWeakMapFor(IRubyObject key) { + if (key instanceof RubyFixnum || key instanceof RubyFloat) { + return valueMap; + } + + return identityMap; + } + @JRubyMethod(name = "[]=") public IRubyObject op_aref(ThreadContext context, IRubyObject key, IRubyObject value) { Ruby runtime = context.runtime; - map.put(key, value); + Map weakMap = getWeakMapFor(key); + weakMap.put(key, value); return runtime.newFixnum(System.identityHashCode(value)); } @JRubyMethod(name = "key?") public IRubyObject key_p(ThreadContext context, IRubyObject key) { - return RubyBoolean.newBoolean(context, map.get(key) != null); + Map weakMap = getWeakMapFor(key); + return RubyBoolean.newBoolean(context, weakMap.get(key) != null); } @JRubyMethod(name = "keys") public IRubyObject keys(ThreadContext context) { return context.runtime.newArrayNoCopy( - map.entrySet() - .stream() + getEntryStream() .filter(entry -> entry.getValue() != null) - .map(entry -> entry.getKey()) + .map(Map.Entry::getKey) .toArray(IRubyObject[]::new)); } + private Stream> getEntryStream() { + return Stream.concat(identityMap.entrySet().stream(), valueMap.entrySet().stream()); + } + @JRubyMethod(name = "values") public IRubyObject values(ThreadContext context) { return context.runtime.newArrayNoCopy( - map.values() - .stream() + getEntryStream() + .map(Map.Entry::getValue) .filter(ref -> ref != null) .toArray(IRubyObject[]::new)); } @JRubyMethod(name = {"length", "size"}) public IRubyObject size(ThreadContext context) { - return context.runtime.newFixnum(map.size()); + return context.runtime.newFixnum(identityMap.size() + valueMap.size()); } @JRubyMethod(name = {"include?", "member?"}) public IRubyObject member_p(ThreadContext context, IRubyObject key) { - return RubyBoolean.newBoolean(context, map.containsKey(key)); + return RubyBoolean.newBoolean(context, getWeakMapFor(key).containsKey(key)); } @JRubyMethod(name = {"each", "each_pair"}) public IRubyObject each(ThreadContext context, Block block) { - map.forEach((key, value) -> { + getEntryStream().forEach((entry) -> { + IRubyObject value = entry.getValue(); if (value != null) { - block.yieldSpecific(context, key, value); + block.yieldSpecific(context, entry.getKey(), value); } }); @@ -292,23 +309,23 @@ public IRubyObject each(ThreadContext context, Block block) { @JRubyMethod(name = "each_key") public IRubyObject each_key(ThreadContext context, Block block) { - for (Map.Entry entry : map.entrySet()) { + getEntryStream().forEach((entry) -> { if (entry.getValue() != null) { block.yieldSpecific(context, entry.getKey()); } - } + }); return this; } @JRubyMethod(name = "each_value") public IRubyObject each_value(ThreadContext context, Block block) { - for (Map.Entry entry : map.entrySet()) { + getEntryStream().forEach((entry) -> { IRubyObject value = entry.getValue(); if (value != null) { block.yieldSpecific(context, value); } - } + }); return this; } @@ -320,7 +337,7 @@ public IRubyObject inspect(ThreadContext context) { RubyString part = inspectPrefix(runtime.getCurrentContext(), metaClass.getRealClass(), inspectHashCode()); int base = part.length(); - map.entrySet().forEach(entry -> { + getEntryStream().forEach(entry -> { if (entry.getValue() != null) { if (part.length() == base) { part.cat(Inspector.COLON_SPACE); @@ -339,6 +356,7 @@ public IRubyObject inspect(ThreadContext context) { return part; } - private final WeakValuedIdentityMap map = new WeakValuedIdentityMap(); + private final WeakValuedIdentityMap identityMap = new WeakValuedIdentityMap<>(); + private final WeakValuedMap valueMap = new WeakValuedMap<>(); } } From 7300edf66d9ae6ff6c55bd82b466c92808b477cc Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Sat, 25 Nov 2023 11:55:33 -0500 Subject: [PATCH 3/5] Do not emit a linenum before node which is side-effect free. This will not advance last processed num so next side effect on same line can still lead to a linenum instr --- core/src/main/java/org/jruby/ir/IRBuilder.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java index 0ceda4fa44b..964cfd2f460 100644 --- a/core/src/main/java/org/jruby/ir/IRBuilder.java +++ b/core/src/main/java/org/jruby/ir/IRBuilder.java @@ -445,13 +445,15 @@ private void determineIfWeNeedLineNumber(Node node) { int currLineNum = node.getLine(); if (currLineNum != lastProcessedLineNum && !(node instanceof NilImplicitNode)) { LineInfo needsCoverage = node.isNewline() ? LineInfo.Coverage : null; + boolean sideEffect = !(node instanceof SideEffectFree); // DefNode will set it's own line number as part of impl but if it is for coverage we emit as instr also. - if (needsCoverage != null && (!(node instanceof DefNode) || coverageMode != 0)) { // Do not emit multiple line number instrs for the same line + if (needsCoverage != null && (!(node instanceof DefNode) && sideEffect || coverageMode != 0)) { // Do not emit multiple line number instrs for the same line needsLineNumInfo = node.isNewline() ? needsCoverage : LineInfo.Backtrace; } // This line is already process either by linenum or by instr which emits its own. - lastProcessedLineNum = currLineNum; + + if (sideEffect) lastProcessedLineNum = currLineNum; } } From ee9a234567dd3f18ca9691b50c2d772f0f81706b Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Mon, 27 Nov 2023 14:26:37 -0500 Subject: [PATCH 4/5] Revert "Do not emit a linenum before node which is side-effect free. This will not advance last processed num so next side effect on same line can still lead to a linenum instr" This reverts commit 7300edf66d9ae6ff6c55bd82b466c92808b477cc. --- core/src/main/java/org/jruby/ir/IRBuilder.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java index 964cfd2f460..0ceda4fa44b 100644 --- a/core/src/main/java/org/jruby/ir/IRBuilder.java +++ b/core/src/main/java/org/jruby/ir/IRBuilder.java @@ -445,15 +445,13 @@ private void determineIfWeNeedLineNumber(Node node) { int currLineNum = node.getLine(); if (currLineNum != lastProcessedLineNum && !(node instanceof NilImplicitNode)) { LineInfo needsCoverage = node.isNewline() ? LineInfo.Coverage : null; - boolean sideEffect = !(node instanceof SideEffectFree); // DefNode will set it's own line number as part of impl but if it is for coverage we emit as instr also. - if (needsCoverage != null && (!(node instanceof DefNode) && sideEffect || coverageMode != 0)) { // Do not emit multiple line number instrs for the same line + if (needsCoverage != null && (!(node instanceof DefNode) || coverageMode != 0)) { // Do not emit multiple line number instrs for the same line needsLineNumInfo = node.isNewline() ? needsCoverage : LineInfo.Backtrace; } // This line is already process either by linenum or by instr which emits its own. - - if (sideEffect) lastProcessedLineNum = currLineNum; + lastProcessedLineNum = currLineNum; } } From 1710cd263caf882a2a1ba71376822122b5a398f8 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Wed, 29 Nov 2023 10:22:59 -0500 Subject: [PATCH 5/5] single pattern not properly working and we had a printf for error string getting created in non-single pattern matches --- core/src/main/java/org/jruby/ir/IRBuilder.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java index 0ceda4fa44b..fbe5481b947 100644 --- a/core/src/main/java/org/jruby/ir/IRBuilder.java +++ b/core/src/main/java/org/jruby/ir/IRBuilder.java @@ -1461,8 +1461,10 @@ private void buildArrayPattern(Label testEnd, Variable result, Variable deconstr label("min_args_check_end", minArgsCheck -> { BIntInstr.Op compareOp = pattern.hasRestArg() ? BIntInstr.Op.GTE : BIntInstr.Op.EQ; addInstr(new BIntInstr(minArgsCheck, compareOp, length, minArgsCount)); - fcall(errorString, buildSelf(), "sprintf", - new FrozenString("%s: %s length mismatch (given %d, expected %d)"), deconstructed, deconstructed, as_fixnum(length), as_fixnum(minArgsCount)); + if (isSinglePattern) { + fcall(errorString, buildSelf(), "sprintf", + new FrozenString("%s: %s length mismatch (given %d, expected %d)"), deconstructed, deconstructed, as_fixnum(length), as_fixnum(minArgsCount)); + } addInstr(new CopyInstr(result, fals())); jump(testEnd); }); @@ -1788,14 +1790,15 @@ public Operand buildPatternCase(PatternCaseNode patternCase) { // build each "when" Variable deconstructed = copy(buildNil()); - for (Node aCase : patternCase.getCases().children()) { + Node[] cases = patternCase.getCases().children(); + boolean isSinglePattern = cases.length == 1; + for (Node aCase : cases) { InNode inNode = (InNode) aCase; Label bodyLabel = getNewLabel(); - boolean isSinglePattern = inNode.isSinglePattern(); - Variable eqqResult = copy(tru()); labels.add(bodyLabel); + buildPatternMatch(eqqResult, deconstructed, inNode.getExpression(), value, false, isSinglePattern, errorString); addInstr(createBranch(eqqResult, tru(), bodyLabel)); bodies.put(bodyLabel, inNode.getBody());