Remove newline node for memory savings and simplicity (~1.1M on empty…

… Rails app)
jruby · Aug 6, 2015 · c293b63 · c293b63
1 parent ca3b0c1
commit c293b63
Showing 5 changed files with 34 additions and 88 deletions.
diff --git a/core/src/main/java/org/jruby/ast/NewlineNode.java b/core/src/main/java/org/jruby/ast/NewlineNode.java
@@ -38,6 +38,8 @@
 import org.jruby.lexer.yacc.ISourcePosition;
 
 /**
+ * Note: This is a dead class but we leave it because people write against Visitor
+ * and we do not want those consumers to break.
  * A new (logical) source code line.
  * This is used to change the value of the ruby interpreter source and line values.
  * There is one such node for each logical line.  Logical line differs
@@ -50,6 +52,7 @@
 public class NewlineNode extends Node {
     private final Node nextNode;
 
+    @Deprecated
     public NewlineNode(ISourcePosition position, Node nextNode) {
         super(position, nextNode.containsVariableAssignment());
 

diff --git a/core/src/main/java/org/jruby/ast/Node.java b/core/src/main/java/org/jruby/ast/Node.java
@@ -59,12 +59,21 @@ public abstract class Node implements ISourcePositionHolder, ParseResult {
     // in IR, we can see that ArrayNode contains an assignment and emit its individual elements differently
     // so that the two values of a end up being different.
     protected boolean containsVariableAssignment;
+    protected boolean newline;
 
     public Node(ISourcePosition position, boolean containsAssignment) {
         this.position = position;
         this.containsVariableAssignment = containsAssignment;
     }
 
+    public void setNewline() {
+        this.newline = true;
+    }
+
+    public boolean isNewline() {
+        return newline;
+    }
+
     /**
      * Location of this node within the source
      */

diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -358,6 +358,17 @@ private void emitEnsureBlocks(IRLoop loop) {
     }
 
     private Operand buildOperand(Node node) throws NotCompilableException {
+        if (node.isNewline()) {
+            int currLineNum = node.getLine();
+            if (currLineNum != _lastProcessedLineNum) { // Do not emit multiple line number instrs for the same line
+                if (RubyInstanceConfig.FULL_TRACE_ENABLED) {
+                    addInstr(new TraceInstr(RubyEvent.LINE, methodNameFor(), getFileName(), currLineNum));
+                }
+                addInstr(manager.newLineNumber(currLineNum));
+                _lastProcessedLineNum = currLineNum;
+            }
+        }
+
         switch (node.getNodeType()) {
             case ALIASNODE: return buildAlias((AliasNode) node);
             case ANDNODE: return buildAnd((AndNode) node);
@@ -416,7 +427,6 @@ private Operand buildOperand(Node node) throws NotCompilableException {
             case MATCHNODE: return buildMatch((MatchNode) node);
             case MODULENODE: return buildModule((ModuleNode) node);
             case MULTIPLEASGNNODE: return buildMultipleAsgn19((MultipleAsgnNode) node);
-            case NEWLINENODE: return buildNewline((NewlineNode) node);
             case NEXTNODE: return buildNext((NextNode) node);
             case NTHREFNODE: return buildNthRef((NthRefNode) node);
             case NILNODE: return buildNil();
@@ -471,26 +481,6 @@ public static IRBuilder topIRBuilder(IRManager manager, IRScope newScope) {
         return new IRBuilder(manager, newScope, null);
     }
 
-    public Node skipOverNewlines(Node n) {
-        if (n.getNodeType() == NodeType.NEWLINENODE) {
-            // Do not emit multiple line number instrs for the same line
-            int currLineNum = n.getLine();
-            if (currLineNum != _lastProcessedLineNum) {
-                if (RubyInstanceConfig.FULL_TRACE_ENABLED) {
-                    addInstr(new TraceInstr(RubyEvent.LINE, methodNameFor(), getFileName(), currLineNum));
-                }
-               addInstr(manager.newLineNumber(currLineNum));
-               _lastProcessedLineNum = currLineNum;
-            }
-        }
-
-        while (n.getNodeType() == NodeType.NEWLINENODE) {
-            n = ((NewlineNode) n).getNextNode();
-        }
-
-        return n;
-    }
-
     public Operand build(Node node) {
         if (node == null) return null;
 
@@ -1370,8 +1360,6 @@ private Operand protectCodeWithRescue(CodeBlock protectedCode, CodeBlock rescueB
     }
 
     public Operand buildGetDefinition(Node node) {
-        node = skipOverNewlines(node);
-
         // FIXME: Do we still have MASGN and MASGN19?
         switch (node.getNodeType()) {
         case CLASSVARASGNNODE: case CLASSVARDECLNODE: case CONSTDECLNODE:
@@ -2544,7 +2532,7 @@ public Operand buildHash(HashNode hashNode) {
     //     --- r is the result of the if expression --
     //
     public Operand buildIf(final IfNode ifNode) {
-        Node actualCondition = skipOverNewlines(ifNode.getCondition());
+        Node actualCondition = ifNode.getCondition();
 
         Variable result;
         Label    falseLabel = getNewLabel();
@@ -2755,10 +2743,6 @@ public Operand buildModule(ModuleNode moduleNode) {
         return processBodyResult;
     }
 
-    public Operand buildNewline(NewlineNode node) {
-        return build(skipOverNewlines(node));
-    }
-
     public Operand buildNext(final NextNode nextNode) {
         IRLoop currLoop = getCurrentLoop();
 
@@ -3170,7 +3154,7 @@ private void buildRescueBodyInternal(RescueBodyNode rescueBodyNode, Variable rv,
 
         // Caught exception case -- build rescue body
         addInstr(new LabelInstr(caughtLabel));
-        Node realBody = skipOverNewlines(rescueBodyNode.getBodyNode());
+        Node realBody = rescueBodyNode.getBodyNode();
         Operand x = build(realBody);
         if (x != U_NIL) { // can be U_NIL if the rescue block has an explicit return
             // Set up node return value 'rv'

diff --git a/core/src/main/java/org/jruby/parser/ParserSupport.java b/core/src/main/java/org/jruby/parser/ParserSupport.java
@@ -172,14 +172,14 @@ protected void getterIdentifierError(ISourcePosition position, String identifier
      *  Wraps node with NEWLINE node.
      *
      *@param node
-     *@return a NewlineNode or null if node is null.
      */
     public Node newline_node(Node node, ISourcePosition position) {
         if (node == null) return null;
 
         configuration.coverLine(position.getLine());
-
-        return node instanceof NewlineNode ? node : new NewlineNode(position, node); 
+        node.setNewline();
+
+        return node;
     }
 
     public Node addRootNode(Node topOfAST) {
@@ -212,9 +212,6 @@ public Node appendToBlock(Node head, Node tail) {
         if (tail == null) return head;
         if (head == null) return tail;
 
-        // Reduces overhead in interp by not set position every single line we encounter.
-        head = compactNewlines(head);
-
         if (!(head instanceof BlockNode)) {
             head = new BlockNode(head.getPosition()).add(head);
         }
@@ -368,9 +365,6 @@ public boolean isBreakStatement(Node node) {
             if (node == null) return false;
 
             switch (node.getNodeType()) {
-            case NEWLINENODE:
-                node = ((NewlineNode) node).getNextNode();
-                continue breakLoop;
             case BREAKNODE: case NEXTNODE: case REDONODE:
             case RETRYNODE: case RETURNNODE:
                 return true;
@@ -392,18 +386,6 @@ public void warningUnlessEOption(ID id, Node node, String message) {
         }
     }
 
-    private Node compactNewlines(Node head) {
-        while (head instanceof NewlineNode) {
-            Node nextNode = ((NewlineNode) head).getNextNode();
-
-            if (!(nextNode instanceof NewlineNode)) {
-                break;
-            }
-            head = nextNode;
-        }
-        return head;
-    }
-
     // logical equivalent to value_expr in MRI
     public boolean checkExpression(Node node) {
         boolean conditional = false;
@@ -429,9 +411,6 @@ public boolean checkExpression(Node node) {
                 conditional = true;
                 node = ((BinaryOperatorNode) node).getSecondNode();
                 break;
-            case NEWLINENODE:
-                node = ((NewlineNode) node).getNextNode();
-                break;
             default: // Node
                 return true;
             }
@@ -472,9 +451,6 @@ public void checkUselessStatement(Node node) {
             if (node == null) return;
 
             switch (node.getNodeType()) {
-            case NEWLINENODE:
-                node = ((NewlineNode) node).getNextNode();
-                continue uselessLoop;
             case CALLNODE: {
                 String name = ((CallNode) node).getName();
 
@@ -606,13 +582,11 @@ private Node cond0(Node node) {
     }
 
     public Node getConditionNode(Node node) {
-        if (node == null) return NilImplicitNode.NIL;
+        Node cond = cond0(node);
 
-        if (node instanceof NewlineNode) {
-            return new NewlineNode(node.getPosition(), cond0(((NewlineNode) node).getNextNode()));
-        } 
+        cond.setNewline();
 
-        return cond0(node);
+        return cond;
     }
 
     /* MRI: range_op */
@@ -621,8 +595,6 @@ private Node getFlipConditionNode(Node node) {
 
         node = getConditionNode(node);
 
-        if (node instanceof NewlineNode) return ((NewlineNode) node).getNextNode();
-
         if (node instanceof FixnumNode) {
             warnUnlessEOption(ID.LITERAL_IN_CONDITIONAL_RANGE, node, "integer literal in conditional range");
             return getOperatorCallNode(node, "==", new GlobalVarNode(node.getPosition(), "$."));
@@ -958,20 +930,9 @@ public Node literal_concat(ISourcePosition position, Node head, Node tail) {
     }
 
     public Node newEvStrNode(ISourcePosition position, Node node) {
-        Node head = node;
-        while (true) {
-            if (node == null) break;
-
-            if (node instanceof StrNode || node instanceof DStrNode || node instanceof EvStrNode) {
-                return node;
-            }
-
-            if (!(node instanceof NewlineNode)) break;
-
-            node = ((NewlineNode) node).getNextNode();
-        }
-
-        return new EvStrNode(position, head);
+        if (node instanceof StrNode || node instanceof DStrNode || node instanceof EvStrNode) return node;
+
+        return new EvStrNode(position, node);
     }
 
     public Node new_yield(ISourcePosition position, Node node) {
@@ -1022,13 +983,6 @@ public RationalNode negateRational(RationalNode rationalNode) {
                                 rationalNode.getDenominator());
     }
 
-    public Node unwrapNewlineNode(Node node) {
-    	if(node instanceof NewlineNode) {
-    		return ((NewlineNode) node).getNextNode();
-    	}
-    	return node;
-    }
-
     private Node checkForNilNode(Node node, ISourcePosition defaultPosition) {
         return (node == null) ? new NilNode(defaultPosition) : node; 
     }

diff --git a/truffle/src/main/java/org/jruby/truffle/translator/BodyTranslator.java b/truffle/src/main/java/org/jruby/truffle/translator/BodyTranslator.java
@@ -1064,10 +1064,6 @@ public RubyNode visitDefinedNode(org.jruby.ast.DefinedNode node) {
 
         org.jruby.ast.Node expressionNode = node.getExpressionNode();
 
-        while (expressionNode instanceof org.jruby.ast.NewlineNode) {
-            expressionNode = ((org.jruby.ast.NewlineNode) expressionNode).getNextNode();
-        }
-
         return new DefinedNode(context, sourceSection, node.getExpressionNode().accept(this));
     }