THRIFT-4712: Improve Performance and Refactor ShortStack
diff --git a/lib/java/README.md b/lib/java/README.md
index cdd4059..0b5f0d8 100644
--- a/lib/java/README.md
+++ b/lib/java/README.md
@@ -170,3 +170,7 @@
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.
+
diff --git a/lib/java/src/org/apache/thrift/ShortStack.java b/lib/java/src/org/apache/thrift/protocol/ShortStack.java
similarity index 74%
rename from lib/java/src/org/apache/thrift/ShortStack.java
rename to lib/java/src/org/apache/thrift/protocol/ShortStack.java
index 4957d1c..9e65930 100644
--- a/lib/java/src/org/apache/thrift/ShortStack.java
+++ b/lib/java/src/org/apache/thrift/protocol/ShortStack.java
@@ -16,45 +16,43 @@
* 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
@@ -62,18 +60,15 @@
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("]>");
diff --git a/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java b/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java
index af145ef..ee05869 100644
--- a/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java
+++ b/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;
diff --git a/lib/java/test/org/apache/thrift/TestShortStack.java b/lib/java/test/org/apache/thrift/TestShortStack.java
deleted file mode 100644
index 07831e5..0000000
--- a/lib/java/test/org/apache/thrift/TestShortStack.java
+++ /dev/null
@@ -1,116 +0,0 @@
-/*
- * 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;
-
-import java.util.Stack;
-
-import junit.framework.TestCase;
-
-public class TestShortStack extends TestCase {
- private static final int NUM_TRIALS = 5;
- private static final int NUM_REPS = 10000000;
-
- public void testOps() throws Exception {
- ShortStack s = new ShortStack(10);
- s.push((short)10);
- s.push((short)11);
- s.push((short)12);
- assertEquals((short)12, s.peek());
- assertEquals((short)12, s.peek());
- assertEquals((short)12, s.pop());
- assertEquals((short)11, s.pop());
- s.push((short)40);
- assertEquals((short)40, s.peek());
- assertEquals((short)40, s.pop());
- assertEquals((short)10, s.peek());
- assertEquals((short)10, s.pop());
- try {
- s.peek();
- fail("should have thrown an exception!");
- } catch (Exception e) {
- // yay
- }
-
- try {
- s.pop();
- fail("should have thrown an exception!");
- } catch (Exception e) {
- // yay
- }
- }
-
- public void testGrow() throws Exception {
- ShortStack s = new ShortStack(1);
- s.push((short)1);
- s.push((short)1);
- s.push((short)1);
- s.push((short)1);
- s.push((short)1);
- }
-
- public static void main(String[] args) throws Exception {
- for (int trial = 0; trial < NUM_TRIALS; trial++) {
- long start = System.currentTimeMillis();
- ShortStack s = new ShortStack(10);
- for (int rep = 0; rep < NUM_REPS; rep++) {
- s.push((short)1);
- s.push((short)11);
- s.push((short)111);
- s.pop();
- s.pop();
- s.push((short)12);
- s.push((short)121);
- s.push((short)1211);
- s.push((short)12111);
- s.pop();
- s.pop();
- s.pop();
- s.pop();
- s.push((short)5);
- s.pop();
- s.pop();
- }
- long end = System.currentTimeMillis();
- System.out.println("ShortStack: " + (end-start));
-
- start = System.currentTimeMillis();
- Stack<Short> stdStack = new Stack<Short>();
- for (int rep = 0; rep < NUM_REPS; rep++) {
- stdStack.push((short)1);
- stdStack.push((short)11);
- stdStack.push((short)111);
- stdStack.pop();
- stdStack.pop();
- stdStack.push((short)12);
- stdStack.push((short)121);
- stdStack.push((short)1211);
- stdStack.push((short)12111);
- stdStack.pop();
- stdStack.pop();
- stdStack.pop();
- stdStack.pop();
- stdStack.push((short)5);
- stdStack.pop();
- stdStack.pop();
- }
- end = System.currentTimeMillis();
- System.out.println("Built-in stack: " + (end-start));
- }
- }
-}
diff --git a/lib/java/test/org/apache/thrift/protocol/TestShortStack.java b/lib/java/test/org/apache/thrift/protocol/TestShortStack.java
new file mode 100644
index 0000000..c8e78ee
--- /dev/null
+++ b/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
+ }
+ }
+}