Make new code path for likely literal keyword argument calls and fcal…

…ls (super is still outstanding). However we implement keyword argument dispatch past this point is still outstanding but the new create will be the point we do it from.
jruby · Aug 10, 2017 · e6c47b8 · e6c47b8
1 parent ca55782
commit e6c47b8
Showing 3 changed files with 110 additions and 26 deletions.
diff --git a/core/src/main/java/org/jruby/ast/HashNode.java b/core/src/main/java/org/jruby/ast/HashNode.java
@@ -76,7 +76,7 @@ public HashNode add(KeyValuePair<Node,Node> pair) {
             containsVariableAssignment = true;
         }
 
-        if (!(pair.getKey() instanceof SymbolNode)) hasOnlySymbolKeys = false;
+        if (!(pair.getKey() instanceof SymbolNode) || pair.getKey() == null) hasOnlySymbolKeys = false;
 
         pairs.add(pair);
 

diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -1,6 +1,5 @@
 package org.jruby.ir;
 
-import org.jcodings.specific.ASCIIEncoding;
 import org.jcodings.specific.USASCIIEncoding;
 import org.jruby.Ruby;
 import org.jruby.RubyInstanceConfig;
@@ -629,13 +628,24 @@ protected Operand buildAttrAssignCallArgs(List<Operand> argsList, Node args, boo
     }
 
     protected Operand[] buildCallArgs(Node args) {
+        return buildCallArgsExcept(args, null);
+    }
+
+    /**
+     * build each argument to a call but if we detect what appears to be a literal simple keyword argument
+     * signature we will pass that along to be excluded in the build here (we will build it separately).
+     * @param args for a call to be built
+     * @param excludeKeywordArg do not build the last one since it is a keyword arg
+     * @return the operands for the call.
+     */
+    protected Operand[] buildCallArgsExcept(Node args, Node excludeKeywordArg) {
         switch (args.getNodeType()) {
             case ARGSCATNODE:
             case ARGSPUSHNODE:
                 return new Operand[] { new Splat(addResultInstr(new BuildSplatInstr(createTemporaryVariable(), build(args), false))) };
             case ARRAYNODE: {
                 Node[] children = ((ListNode) args).children();
-                int numberOfArgs = children.length;
+                int numberOfArgs = children.length - (excludeKeywordArg != null ? 1 : 0);
                 Operand[] builtArgs = new Operand[numberOfArgs];
                 boolean hasAssignments = args.containsVariableAssignment();
 
@@ -1026,7 +1036,6 @@ private void receiveBreakException(Operand block, final CallInstr callInstr) {
     }
 
     public Operand buildCall(Variable result, CallNode callNode) {
-        Node callArgsNode = callNode.getArgsNode();
         Node receiverNode = callNode.getReceiverNode();
 
         // Frozen string optimization: check for "string".freeze
@@ -1063,18 +1072,22 @@ public Operand buildCall(Variable result, CallNode callNode) {
             addInstr(new BNilInstr(lazyLabel, receiver));
         }
 
-        Operand[] args = setupCallArgs(callArgsNode);
-        Operand block = setupCallClosure(callNode.getIterNode());
-
-        CallInstr callInstr = CallInstr.create(scope, result, callNode.getName(), receiver, args, block);
+        HashNode keywordArgs = getPossibleKeywordArgument(callNode.getArgsNode());
 
-        // This is to support the ugly Proc.new with no block, which must see caller's frame
-        if ( callNode.getName().equals("new") &&
-             receiverNode instanceof ConstNode &&
-             ((ConstNode)receiverNode).getName().equals("Proc")) {
-            callInstr.setProcNew(true);
+        CallInstr callInstr;
+        Operand block;
+        if (keywordArgs != null) {
+            Operand[] args = buildCallArgsExcept(callNode.getArgsNode(), keywordArgs);
+            List<KeyValuePair<Operand, Operand>> kwargs = buildKeywordArguments(keywordArgs);
+            block = setupCallClosure(callNode.getIterNode());
+            callInstr = CallInstr.createWithKwargs(scope, CallType.NORMAL, result, callNode.getName(), receiver, args, block, kwargs);
+        } else {
+            Operand[] args = setupCallArgs(callNode.getArgsNode());
+            block = setupCallClosure(callNode.getIterNode());
+            callInstr = CallInstr.create(scope, result, callNode.getName(), receiver, args, block);
         }
 
+        determineIfProcNew(receiverNode, callNode.getName(), callInstr);
         receiveBreakException(block, callInstr);
 
         if (callNode.isLazy()) {
@@ -1087,6 +1100,47 @@ public Operand buildCall(Variable result, CallNode callNode) {
         return result;
     }
 
+    private List<KeyValuePair<Operand, Operand>> buildKeywordArguments(HashNode keywordArgs) {
+        List<KeyValuePair<Operand, Operand>> kwargs = new ArrayList<>();
+        for (KeyValuePair<Node, Node> pair: keywordArgs.getPairs()) {
+            kwargs.add(new KeyValuePair<Operand, Operand>((Symbol) build(pair.getKey()), build(pair.getValue())));
+        }
+        return kwargs;
+    }
+
+    private void determineIfProcNew(Node receiverNode, String name, CallInstr callInstr) {
+        // This is to support the ugly Proc.new with no block, which must see caller's frame
+        if (name.equals("new") && receiverNode instanceof ConstNode && ((ConstNode)receiverNode).getName().equals("Proc")) {
+            callInstr.setProcNew(true);
+        }
+    }
+
+    /*
+     * This will ignore complexity of a hash which contains restkwargs and only return simple
+     * last argument which happens to be all key symbol hashnode which is not empty
+     */
+    private HashNode getPossibleKeywordArgument(Node argsNode) {
+        // Block pass wraps itself around the main args list so don't hold that against it.
+        if (argsNode instanceof BlockPassNode) {
+            return null; //getPossibleKeywordArgument(((BlockPassNode) argsNode).getArgsNode());
+        }
+
+        if (argsNode instanceof ArrayNode) {
+            ArrayNode argsList = (ArrayNode) argsNode;
+
+            if (argsList.isEmpty()) return null;
+
+            Node lastNode = argsList.getLast();
+
+            if (lastNode instanceof HashNode) {
+                HashNode hash = (HashNode) lastNode;
+                if (hash.hasOnlySymbolKeys() && !hash.isEmpty()) return (HashNode) lastNode;
+            }
+        }
+
+        return null;
+    }
+
     public Operand buildCase(CaseNode caseNode) {
         // scan all cases to see if we have a homogeneous literal case/when
         NodeType seenType = null;
@@ -2671,28 +2725,41 @@ public Operand buildFalse() {
 
     public Operand buildFCall(Variable result, FCallNode fcallNode) {
         Node      callArgsNode = fcallNode.getArgsNode();
-        Operand[] args         = setupCallArgs(callArgsNode);
-        Operand   block        = setupCallClosure(fcallNode.getIterNode());
 
         if (result == null) result = createTemporaryVariable();
 
-        determineIfMaybeUsingMethod(fcallNode.getName(), args);
-
-        // We will stuff away the iters AST source into the closure in the hope we can convert
-        // this closure to a method.
-        if (fcallNode.getName().equals("define_method") && block instanceof WrappedIRClosure) {
-            IRClosure closure = ((WrappedIRClosure) block).getClosure();
+        HashNode keywordArgs = getPossibleKeywordArgument(fcallNode.getArgsNode());
 
-            // To convert to a method we need its variable scoping to appear like a normal method.
-            if (!closure.getFlags().contains(IRFlags.ACCESS_PARENTS_LOCAL_VARIABLES) &&
-                    fcallNode.getIterNode() instanceof IterNode) {
-                closure.setSource((IterNode) fcallNode.getIterNode());
+        CallInstr callInstr;
+        Operand block;
+        if (keywordArgs != null) {
+            Operand[] args = buildCallArgsExcept(fcallNode.getArgsNode(), keywordArgs);
+            List<KeyValuePair<Operand, Operand>> kwargs = buildKeywordArguments(keywordArgs);
+            block = setupCallClosure(fcallNode.getIterNode());
+            callInstr = CallInstr.createWithKwargs(scope, CallType.FUNCTIONAL, result, fcallNode.getName(), buildSelf(), args, block, kwargs);
+        } else {
+            Operand[] args         = setupCallArgs(callArgsNode);
+            block        = setupCallClosure(fcallNode.getIterNode());
+            determineIfMaybeUsingMethod(fcallNode.getName(), args);
+
+            // We will stuff away the iters AST source into the closure in the hope we can convert
+            // this closure to a method.
+            if (fcallNode.getName().equals("define_method") && block instanceof WrappedIRClosure) {
+                IRClosure closure = ((WrappedIRClosure) block).getClosure();
+
+                // To convert to a method we need its variable scoping to appear like a normal method.
+                if (!closure.getFlags().contains(IRFlags.ACCESS_PARENTS_LOCAL_VARIABLES) &&
+                        fcallNode.getIterNode() instanceof IterNode) {
+                    closure.setSource((IterNode) fcallNode.getIterNode());
+                }
             }
+
+            callInstr = CallInstr.create(scope, CallType.FUNCTIONAL, result, fcallNode.getName(), buildSelf(), args, block);
         }
 
         determineIfWeNeedLineNumber(fcallNode); // buildOperand for fcall was papered over by args operand building so we check once more.
-        CallInstr callInstr = CallInstr.create(scope, CallType.FUNCTIONAL, result, fcallNode.getName(), buildSelf(), args, block);
         receiveBreakException(block, callInstr);
+
         return result;
     }
 

diff --git a/core/src/main/java/org/jruby/ir/instructions/CallInstr.java b/core/src/main/java/org/jruby/ir/instructions/CallInstr.java
@@ -10,19 +10,36 @@
 import org.jruby.ir.instructions.specialized.OneOperandArgNoBlockCallInstr;
 import org.jruby.ir.instructions.specialized.TwoOperandArgNoBlockCallInstr;
 import org.jruby.ir.instructions.specialized.ZeroOperandArgNoBlockCallInstr;
+import org.jruby.ir.operands.Hash;
 import org.jruby.ir.operands.Operand;
 import org.jruby.ir.operands.Variable;
 import org.jruby.ir.persistence.IRReaderDecoder;
 import org.jruby.ir.persistence.IRWriterEncoder;
 import org.jruby.ir.transformations.inlining.CloneInfo;
 import org.jruby.runtime.CallType;
+import org.jruby.util.KeyValuePair;
+
+import java.util.ArrayList;
+import java.util.List;
 
 /*
  * args field: [self, receiver, *args]
  */
 public class CallInstr extends CallBase implements ResultInstr {
     protected transient Variable result;
 
+    public static CallInstr createWithKwargs(IRScope scope, CallType callType, Variable result, String name,
+                                             Operand receiver, Operand[] args, Operand closure,
+                                             List<KeyValuePair<Operand, Operand>> kwargs) {
+        // FIXME: This is obviously total nonsense but this will be on an optimized path and we will not be constructing
+        // a new hash like this unless the eventual caller needs an ordinary hash.
+        Operand[] newArgs = new Operand[args.length + 1];
+        System.arraycopy(args, 0, newArgs, 0, args.length);
+        newArgs[args.length] = new Hash(kwargs, true);
+
+        return create(scope, callType, result, name, receiver, newArgs, closure);
+    }
+
     public static CallInstr create(IRScope scope, Variable result, String name, Operand receiver, Operand[] args, Operand closure) {
         return create(scope, CallType.NORMAL, result, name, receiver, args, closure);
     }