Repository: pgjdbc/pgjdbc
Pull request #1232 (2018-06-30 15:53:05)

perf: avoid string allocation for oid/rows parsing in command tag
This saves even more string allocation in `interpretCommandStatus`

Allocation profiling results are as follows (`UpdateBatch` benchmark):

```
Java          1.8u152  9.0.4 Units
master          50177  51776 bytes/op
#1219           30177  26976 bytes/op
matcher reuse   30177  29376 bytes/op
toLong          20576  19776 bytes/op
CommandCoParser 23776  19776 bytes/op
```

codecov-io: # [Codecov](https://codecov.io/gh/pgjdbc/pgjdbc/pull/1232?src=pr&el=h1) Report
> Merging [#1232](https://codecov.io/gh/pgjdbc/pgjdbc/pull/1232?src=pr&el=desc) into [master](https://codecov.io/gh/pgjdbc/pgjdbc/commit/ba360f731cb9a2eca9924b700cee234aba572fe5?src=pr&el=desc) will **decrease** coverage by `0.32%`.
> The diff coverage is `68.75%`.



```diff
@@             Coverage Diff              @@
##             master    #1232      +/-   ##
============================================
- Coverage     68.71%   68.38%   -0.33%     
- Complexity     3832     3872      +40     
============================================
  Files           173      173              
  Lines         15994    16267     +273     
  Branches       2602     2679      +77     
============================================
+ Hits          10990    11125     +135     
- Misses         3775     3891     +116     
- Partials       1229     1251      +22
```

vlsi: The logic should be moved into `Parser` (so the test could be added)
bokken: @vlsi , How about a method on Parser which returns a 
```
public static final class StatusResult {
    public String getCommand();
    public long getOID();
    public long getRowCount();
}
public static StatusResult parseStatus(String status) throws NumberFormatException {
    ...
}
```
vlsi: Well, the idea here was to avoid memory allocation, and `StatusResult` looks like a step back.

bokken: IMO there is value in readability/testability. Trading ~100 bytes for code that lends itself to very straightforward unit testing seems a reasonable trade.
To avoid creating an object every time, it could return null in the case there are neither row nor oid info available.
davecramer: well a less than clear way to return it is in a byte buffer that holds both the oid and row count but that is certainly not a step forward in readability of robustness of the code. 

bokken: While looking at this, I have been puzzled that while the row count and oid are parsed out, the handler is still given the raw status with those longs present, rather than the command and number separated. If this truly is the desired state for the foreseeable future, something like this could be done:

```
public static void parseStatusNumbers(String status, long[] nbrs) throws NumberFormatException {
    ...
    nbrs[0] = oid;
    nbrs[1] = count;
}
```

In the `QueryExecutorImpl`, it could maintain a static thread local long[] that is reused across instances and invocations.
vlsi: Well, I might need to check how `StatusResult` would perform.

At the end of the day, there might be a per-QueryExecutor instance of `StatusResult`.
vlsi: I think it is the way to go

Patch:
diff --git a/pgjdbc/src/main/java/org/postgresql/core/CommandCompleteParser.java b/pgjdbc/src/main/java/org/postgresql/core/CommandCompleteParser.java
new file mode 100644
index 00000000..a4e52b31
--- /dev/null
+++ b/pgjdbc/src/main/java/org/postgresql/core/CommandCompleteParser.java
@@ -0,0 +1,108 @@
+/*
+ * Copyright (c) 2018, PostgreSQL Global Development Group
+ * See the LICENSE file in the project root for more information.
+ */
+
+package org.postgresql.core;
+
+import org.postgresql.util.GT;
+import org.postgresql.util.PSQLException;
+import org.postgresql.util.PSQLState;
+
+/**
+ * Parses {@code oid} and {@code rows} from a {@code CommandComplete (B)} message (end of Execute).
+ */
+public final class CommandCompleteParser {
+  private long oid;
+  private long rows;
+
+  public CommandCompleteParser() {
+  }
+
+  public long getOid() {
+    return oid;
+  }
+
+  public long getRows() {
+    return rows;
+  }
+
+  void set(long oid, long rows) {
+    this.oid = oid;
+    this.rows = rows;
+  }
+
+  /**
+   * Parses {@code CommandComplete (B)} message.
+   * Status is in the format of "COMMAND OID ROWS" where both 'OID' and 'ROWS' are optional
+   * and COMMAND can have spaces within it, like CREATE TABLE.
+   *
+   * @param status COMMAND OID ROWS message
+   * @throws PSQLException in case the status cannot be parsed
+   */
+  public void parse(String status) throws PSQLException {
+    // Assumption: command neither starts nor ends with a digit
+    if (!Parser.isDigitAt(status, status.length() - 1)) {
+      set(0, 0);
+      return;
+    }
+
+    // Scan backwards, while searching for a maximum of two number groups
+    //   COMMAND OID ROWS
+    //   COMMAND ROWS
+    long oid = 0;
+    long rows = 0;
+    try {
+      int lastSpace = status.lastIndexOf(' ');
+      // Status ends with a digit => it is ROWS
+      if (Parser.isDigitAt(status, lastSpace + 1)) {
+        rows = Parser.parseLong(status, lastSpace + 1, status.length());
+
+        if (Parser.isDigitAt(status, lastSpace - 1)) {
+          int penultimateSpace = status.lastIndexOf(' ', lastSpace - 1);
+          if (Parser.isDigitAt(status, penultimateSpace + 1)) {
+            oid = Parser.parseLong(status, penultimateSpace + 1, lastSpace);
+          }
+        }
+      }
+    } catch (NumberFormatException e) {
+      // This should only occur if the oid or rows are out of 0..Long.MAX_VALUE range
+      throw new PSQLException(
+          GT.tr("Unable to parse the count in command completion tag: {0}.", status),
+          PSQLState.CONNECTION_FAILURE, e);
+    }
+    set(oid, rows);
+  }
+
+  @Override
+  public String toString() {
+    return "CommandStatus{"
+        + "oid=" + oid
+        + ", rows=" + rows
+        + '}';
+  }
+
+  @Override
+  public boolean equals(Object o) {
+    if (this == o) {
+      return true;
+    }
+    if (o == null || getClass() != o.getClass()) {
+      return false;
+    }
+
+    CommandCompleteParser that = (CommandCompleteParser) o;
+
+    if (oid != that.oid) {
+      return false;
+    }
+    return rows == that.rows;
+  }
+
+  @Override
+  public int hashCode() {
+    int result = (int) (oid ^ (oid >>> 32));
+    result = 31 * result + (int) (rows ^ (rows >>> 32));
+    return result;
+  }
+}
diff --git a/pgjdbc/src/main/java/org/postgresql/core/Parser.java b/pgjdbc/src/main/java/org/postgresql/core/Parser.java
index 5cd2dfb2..c017de0b 100644
--- a/pgjdbc/src/main/java/org/postgresql/core/Parser.java
+++ b/pgjdbc/src/main/java/org/postgresql/core/Parser.java
@@ -691,6 +691,26 @@ public class Parser {
         && (query[offset + 5] | 32) == 's';
   }
 
+  /**
+   * Faster version of {@link Long#parseLong(String)} when parsing a substring is required
+   *
+   * @param s string to parse
+   * @param beginIndex begin index
+   * @param endIndex end index
+   * @return long value
+   */
+  public static long parseLong(String s, int beginIndex, int endIndex) {
+    // Fallback to default implementation in case the string is long
+    if (endIndex - beginIndex > 16) {
+      return Long.parseLong(s.substring(beginIndex, endIndex));
+    }
+    long res = digitAt(s, beginIndex);
+    for (beginIndex++; beginIndex < endIndex; beginIndex++) {
+      res = res * 10 + digitAt(s, beginIndex);
+    }
+    return res;
+  }
+
   /**
    * Parse string to check presence of WITH keyword regardless of case.
    *
@@ -725,6 +745,31 @@ public class Parser {
         && (query[offset + 1] | 32) == 's';
   }
 
+  /**
+   * Returns true if a given string {@code s} has digit at position {@code pos}.
+   * @param s input string
+   * @param pos position (0-based)
+   * @return true if input string s has digit at position pos
+   */
+  public static boolean isDigitAt(String s, int pos) {
+    return pos > 0 && pos < s.length() && Character.isDigit(s.charAt(pos));
+  }
+
+  /**
+   * Converts digit at position {@code pos} in string {@code s} to integer or throws.
+   * @param s input string
+   * @param pos position (0-based)
+   * @return integer value of a digit at position pos
+   * @throws NumberFormatException if character at position pos is not an integer
+   */
+  public static int digitAt(String s, int pos) {
+    int c = s.charAt(pos) - '0';
+    if (c < 0 || c > 9) {
+      throw new NumberFormatException("Input string: \"" + s + "\", position: " + pos);
+    }
+    return c;
+  }
+
   /**
    * @param c character
    * @return true if the character is a whitespace character as defined in the backend's parser
diff --git a/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java b/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
index edc504db..62030e50 100644
--- a/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
+++ b/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
@@ -10,6 +10,7 @@ import org.postgresql.PGProperty;
 import org.postgresql.copy.CopyIn;
 import org.postgresql.copy.CopyOperation;
 import org.postgresql.copy.CopyOut;
+import org.postgresql.core.CommandCompleteParser;
 import org.postgresql.core.Encoding;
 import org.postgresql.core.EncodingPredictor;
 import org.postgresql.core.Field;
@@ -63,9 +64,6 @@ import java.util.Set;
 import java.util.TimeZone;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
-
 
 /**
  * QueryExecutor implementation for the V3 protocol.
@@ -73,7 +71,6 @@ import java.util.regex.Pattern;
 public class QueryExecutorImpl extends QueryExecutorBase {
 
   private static final Logger LOGGER = Logger.getLogger(QueryExecutorImpl.class.getName());
-  private static final Pattern COMMAND_COMPLETE_PATTERN = Pattern.compile("^([A-Za-z]++)(?: (\\d++))?+(?: (\\d++))?+$");
 
   /**
    * TimeZone of the current connection (TimeZone backend parameter).
@@ -122,6 +119,11 @@ public class QueryExecutorImpl extends QueryExecutorBase {
 
   private final ReplicationProtocol replicationProtocol;
 
+  /**
+   * {@code CommandComplete(B)} messages are quite common, so we reuse instance to parse those
+   */
+  private final CommandCompleteParser commandCompleteParser = new CommandCompleteParser();
+
   public QueryExecutorImpl(PGStream pgStream, String user, String database,
       int cancelSignalTimeout, Properties info) throws SQLException, IOException {
     super(pgStream, user, database, cancelSignalTimeout, info);
@@ -2469,31 +2471,15 @@ public class QueryExecutorImpl extends QueryExecutorBase {
   }
 
   private void interpretCommandStatus(String status, ResultHandler handler) {
-    long oid = 0;
-    long count = 0;
-    Matcher matcher = COMMAND_COMPLETE_PATTERN.matcher(status);
-    if (matcher.matches()) {
-      // String command = matcher.group(1);
-      String group2 = matcher.group(2);
-      String group3 = matcher.group(3);
-      try {
-        if (group3 != null) {
-          // COMMAND OID ROWS
-          oid = Long.parseLong(group2);
-          count = Long.parseLong(group3);
-        } else if (group2 != null) {
-          // COMMAND ROWS
-          count = Long.parseLong(group2);
-        }
-      } catch (NumberFormatException e) {
-        // As we're performing a regex validation prior to parsing, this should only
-        // occurr if the oid or count are out of range.
-        handler.handleError(new PSQLException(
-            GT.tr("Unable to parse the count in command completion tag: {0}.", status),
-            PSQLState.CONNECTION_FAILURE));
-        return;
-      }
+    try {
+      commandCompleteParser.parse(status);
+    } catch (SQLException e) {
+      handler.handleError(e);
+      return;
     }
+    long oid = commandCompleteParser.getOid();
+    long count = commandCompleteParser.getRows();
+
     int countAsInt = 0;
     if (count > Integer.MAX_VALUE) {
       // If command status indicates that we've modified more than Integer.MAX_VALUE rows


Test patch:
diff --git a/pgjdbc/src/test/java/org/postgresql/core/CommandCompleteParserNegativeTest.java b/pgjdbc/src/test/java/org/postgresql/core/CommandCompleteParserNegativeTest.java
new file mode 100644
index 00000000..0e6f82dd
--- /dev/null
+++ b/pgjdbc/src/test/java/org/postgresql/core/CommandCompleteParserNegativeTest.java
@@ -0,0 +1,49 @@
+/*
+ * Copyright (c) 2018, PostgreSQL Global Development Group
+ * See the LICENSE file in the project root for more information.
+ */
+
+package org.postgresql.core;
+
+import org.postgresql.util.PSQLException;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.Arrays;
+
+@RunWith(Parameterized.class)
+public class CommandCompleteParserNegativeTest {
+
+  @Parameterized.Parameter(0)
+  public String input;
+
+  @Parameterized.Parameters(name = "input={0}")
+  public static Iterable<Object[]> data() {
+    return Arrays.asList(new Object[][]{
+        {"SELECT 0_0 42"},
+        {"SELECT 42 0_0"},
+        {"SELECT 0_0 0_0"},
+    });
+  }
+
+  @Test
+  public void run() throws PSQLException {
+    CommandCompleteParser parser = new CommandCompleteParser();
+    try {
+      parser.parse(input);
+      Assert.fail("CommandCompleteParser should throw NumberFormatException for " + input);
+    } catch (PSQLException e) {
+      Throwable cause = e.getCause();
+      if (cause == null) {
+        throw e;
+      }
+      if (!(cause instanceof NumberFormatException)) {
+        throw e;
+      }
+      // NumerFormatException is expected
+    }
+  }
+}
diff --git a/pgjdbc/src/test/java/org/postgresql/core/CommandCompleteParserTest.java b/pgjdbc/src/test/java/org/postgresql/core/CommandCompleteParserTest.java
new file mode 100644
index 00000000..ce3878d3
--- /dev/null
+++ b/pgjdbc/src/test/java/org/postgresql/core/CommandCompleteParserTest.java
@@ -0,0 +1,58 @@
+/*
+ * Copyright (c) 2018, PostgreSQL Global Development Group
+ * See the LICENSE file in the project root for more information.
+ */
+
+package org.postgresql.core;
+
+import org.postgresql.util.PSQLException;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.Arrays;
+
+@RunWith(Parameterized.class)
+public class CommandCompleteParserTest {
+
+  @Parameterized.Parameter(0)
+  public String input;
+  @Parameterized.Parameter(1)
+  public long oid;
+  @Parameterized.Parameter(2)
+  public long rows;
+
+  @Parameterized.Parameters(name = "input={0}, oid={1}, rows={2}")
+  public static Iterable<Object[]> data() {
+    return Arrays.asList(new Object[][]{
+        {"SELECT 0", 0, 0},
+        {"SELECT -42", 0, 0},
+        {"SELECT", 0, 0},
+        {"", 0, 0},
+        {"A", 0, 0},
+        {"SELECT 42", 0, 42},
+        {"UPDATE 43 42", 43, 42},
+        {"UPDATE 43 " + Long.MAX_VALUE, 43, Long.MAX_VALUE},
+        {"UPDATE " + Long.MAX_VALUE + " " + Long.MAX_VALUE, Long.MAX_VALUE, Long.MAX_VALUE},
+        {"UPDATE " + (Long.MAX_VALUE / 10) + " " + (Long.MAX_VALUE / 10), (Long.MAX_VALUE / 10),
+            (Long.MAX_VALUE / 10)},
+        {"UPDATE " + (Long.MAX_VALUE / 100) + " " + (Long.MAX_VALUE / 100), (Long.MAX_VALUE / 100),
+            (Long.MAX_VALUE / 100)},
+        {"CREATE TABLE " + (Long.MAX_VALUE / 100) + " " + (Long.MAX_VALUE / 100),
+            (Long.MAX_VALUE / 100), (Long.MAX_VALUE / 100)},
+        {"CREATE TABLE", 0, 0},
+        {"CREATE OR DROP OR DELETE TABLE 42", 0, 42},
+    });
+  }
+
+  @Test
+  public void run() throws PSQLException {
+    CommandCompleteParser expected = new CommandCompleteParser();
+    CommandCompleteParser actual = new CommandCompleteParser();
+    expected.set(oid, rows);
+    actual.parse(input);
+    Assert.assertEquals(input, expected, actual);
+  }
+}
diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/Jdbc2TestSuite.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/Jdbc2TestSuite.java
index 03ac93f8..f1e09d47 100644
--- a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/Jdbc2TestSuite.java
+++ b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/Jdbc2TestSuite.java
@@ -5,6 +5,8 @@
 
 package org.postgresql.test.jdbc2;
 
+import org.postgresql.core.CommandCompleteParserNegativeTest;
+import org.postgresql.core.CommandCompleteParserTest;
 import org.postgresql.core.OidToStringTest;
 import org.postgresql.core.OidValueOfTest;
 import org.postgresql.core.ParserTest;
@@ -78,6 +80,8 @@ import org.junit.runners.Suite;
         TimezoneCachingTest.class,
         ParserTest.class,
         ReturningParserTest.class,
+        CommandCompleteParserTest.class,
+        CommandCompleteParserNegativeTest.class,
 
         OidToStringTest.class,
         OidValueOfTest.class,