THRIFT-4712: Improve Performance and Refactor ShortStack

lib/java/README.md

@@ -170,3 +170,7 @@ The access modifier of the AutoExpandingBuffer class has been changed from
 public to default (package) and will no longer be accessible by third-party
 libraries.
 
+The access modifier of the ShortStack class has been changed from
+public to default (package) and will no longer be accessible by third-party
+libraries.
+

lib/java/src/org/apache/thrift/ShortStack.java → lib/java/src/org/apache/thrift/protocol/ShortStack.java

@@ -16,64 +16,59 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.thrift;
+package org.apache.thrift.protocol;
+
+import java.util.Arrays;
 
 /**
  * ShortStack is a short-specific Stack implementation written for the express
  * purpose of very fast operations on TCompactProtocol's field id stack. This
  * implementation performs at least 10x faster than java.util.Stack.
  */
-public class ShortStack {
+class ShortStack {
 
   private short[] vector;
-  private int top = -1;
+
+  /** Always points to the next location */
+  private int top = 0;
 
   public ShortStack(int initialCapacity) {
     vector = new short[initialCapacity];
   }
 
   public short pop() {
-    return vector[top--];
+    return vector[--top];
   }
 
   public void push(short pushed) {
-    if (vector.length == top + 1) {
+    if (vector.length == top) {
       grow();
     }
-    vector[++top] = pushed;
+    vector[top++] = pushed;
   }
 
   private void grow() {
-    short[] newVector = new short[vector.length * 2];
-    System.arraycopy(vector, 0, newVector, 0, vector.length);
-    vector = newVector;
-  }
-
-  public short peek() {
-    return vector[top];
+    vector = Arrays.copyOf(vector, vector.length << 1);
   }
 
   public void clear() {
-    top = -1;
+    top = 0;
   }
 
   @Override
   public String toString() {
     StringBuilder sb = new StringBuilder();
     sb.append("<ShortStack vector:[");
     for (int i = 0; i < vector.length; i++) {
+      boolean isTop = (i == (top - 1));
+      short value = vector[i];
       if (i != 0) {
-        sb.append(" ");
+        sb.append(' ');
       }
-
-      if (i == top) {
-        sb.append(">>");
-      }
-
-      sb.append(vector[i]);
-
-      if (i == top) {
-        sb.append("<<");
+      if (isTop) {
+        sb.append(">>").append(value).append("<<");
+      } else {
+        sb.append(value);
       }
     }
     sb.append("]>");

lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java

@@ -24,7 +24,6 @@
 import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
 
-import org.apache.thrift.ShortStack;
 import org.apache.thrift.TException;
 import org.apache.thrift.transport.TTransport;
 

lib/java/test/org/apache/thrift/protocol/TestShortStack.java

@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.thrift.protocol;
+
+import junit.framework.TestCase;
+
+public class TestShortStack extends TestCase {
+
+  public void testOps() throws Exception {
+    ShortStack s = new ShortStack(1);
+    s.push((short)10);
+    s.push((short)11);
+    s.push((short)12);
+    assertEquals((short)12, s.pop());
+    assertEquals((short)11, s.pop());
+    s.push((short)40);
+    assertEquals((short)40, s.pop());
+    assertEquals((short)10, s.pop());
+    try {
+      s.pop();
+      fail("should have thrown an exception!");
+    } catch (Exception e) {
+      // yay
+    }
+  }
+}