New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: avoid string allocation for oid/rows parsing in command tag #1232

Merged
merged 14 commits into from Jul 10, 2018

Conversation

Projects
None yet
5 participants
@vlsi
Member

vlsi commented Jun 30, 2018

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

@vlsi vlsi force-pushed the vlsi:command_tag_perf branch from 16a3f84 to f26e899 Jun 30, 2018

@vlsi vlsi added this to the 42.2.3 milestone Jun 30, 2018

@vlsi vlsi added the needs-review label Jun 30, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Jun 30, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@08631cc). Click here to learn what that means.
The diff coverage is 69.09%.

@@            Coverage Diff            @@
##             master    #1232   +/-   ##
=========================================
  Coverage          ?   68.71%           
  Complexity        ?     3850           
=========================================
  Files             ?      174           
  Lines             ?    16035           
  Branches          ?     2614           
=========================================
  Hits              ?    11019           
  Misses            ?     3779           
  Partials          ?     1237
@bokken

bokken approved these changes Jun 30, 2018

}
private static long toLong(String s, int beginIndex, int endIndex) {
if (endIndex > 8 && !isDigitAt(s, beginIndex)) {

This comment has been minimized.

@bokken

bokken Jun 30, 2018

Member

Perhaps a comment explaining why this if check and use of Long.parseLong.

// Scan backwards, while searching for a maximum of two number groups
// COMMAND OID ROWS
// COMMAND ROWS
// Assumption: command does not contain digits

This comment has been minimized.

@bokken

bokken Jun 30, 2018

Member

Is it a valid assumption that no command will have a number (like end in 2 for a second/different behavior)?

* @return long value
*/
private static long toLong(String s, int beginIndex, int endIndex) {
// Fallback to default implementation in case the string is long or the first digi

This comment has been minimized.

@bokken

bokken Jun 30, 2018

Member

I’m guessing the long string is to better handle overflow. Why do we want default implementation for first char not a digit? Don’t both result in number format exception?

This comment has been minimized.

@vlsi

vlsi Jun 30, 2018

Member

isDigit was not needed there. The idea is just delegate large strings to default implementation to avoid dealing with things like MAX_LONG, etc.

This comment has been minimized.

@vlsi

vlsi Jun 30, 2018

Member

Just one more thing: I assume all the numbers are positive, so there cannot be something like -42

@bokken

bokken approved these changes Jun 30, 2018

@vlsi vlsi referenced this pull request Jul 2, 2018

Merged

Release notes for 42.2.3 #1238

@vlsi

This comment has been minimized.

Member

vlsi commented Jul 2, 2018

The logic should be moved into Parser (so the test could be added)

@vlsi

This comment has been minimized.

Member

vlsi commented Jul 2, 2018

Eh, the result of "COMMAND OID ROWS" is a pair of oid/count, and there's no clear way to return that from Parser.

I'm not particularly fond of going with new OidRows(oid, rows) here, so alternative option is to have Parser.parseCommandStatus(String statusMessage, ResultHandler handler).
The third option is just keep parsing in the QueryExecutorImpl

@bokken

This comment has been minimized.

Member

bokken commented Jul 2, 2018

@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

This comment has been minimized.

Member

vlsi commented Jul 3, 2018

Well, the idea here was to avoid memory allocation, and StatusResult looks like a step back.

@bokken

This comment has been minimized.

Member

bokken commented Jul 3, 2018

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

This comment has been minimized.

Member

davecramer commented Jul 3, 2018

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

This comment has been minimized.

Member

bokken commented Jul 3, 2018

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

This comment has been minimized.

Member

vlsi commented Jul 3, 2018

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

This comment has been minimized.

Member

vlsi commented Jul 3, 2018

I think it is the way to go

}
}
} catch (NumberFormatException e) {
// As we're have performed a isDigit check prior to parsing, this should only

This comment has been minimized.

@davecramer

davecramer Jul 3, 2018

Member

As we have ?

This comment has been minimized.

@vlsi

vlsi Jul 3, 2018

Member

Does "performed a isDigit check" sound good?
Should it be "an isDigit"?

This comment has been minimized.

@davecramer

davecramer Jul 3, 2018

Member

an isDigit is technically correct

This comment has been minimized.

@bokken

bokken Jul 3, 2018

Member

this comment isn't completely accurate. we have not done an isDigit call for each char we are parsing. We have definitely checked the first char and possibly have checked the last char.

// COMMAND OID ROWS
// COMMAND ROWS
long oid = 0;
long count = 0;

This comment has been minimized.

@davecramer

davecramer Jul 3, 2018

Member

rows would be a better name for this variable?

/**
* Parses {@code oid} and {@code rows} from a {@code CommandComplete (B)} message (end of Execute).
*/
public class CommandCompleteParser {

This comment has been minimized.

@bokken

bokken Jul 4, 2018

Member

Final?

return rows;
}
public void set(long oid, long rows) {

This comment has been minimized.

@bokken

bokken Jul 4, 2018

Member

Does this need to be public?

This comment has been minimized.

@vlsi

vlsi Jul 4, 2018

Member

for testing

This comment has been minimized.

@bokken

bokken Jul 4, 2018

Member

If test is in same package, visibility of this method could be dropped to default

@bokken

bokken approved these changes Jul 4, 2018

@bokken

bokken approved these changes Jul 4, 2018

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);

This comment has been minimized.

@bokken

bokken Jul 4, 2018

Member

Should return here, right?

This comment has been minimized.

@vlsi

vlsi Jul 4, 2018

Member

right

This comment has been minimized.

@davecramer

davecramer Jul 4, 2018

Member

apparently there isn't a test case for this :)

This comment has been minimized.

@vlsi

vlsi Jul 4, 2018

Member

apparently, there is:

It just does not hurt

This comment has been minimized.

@bokken

bokken Jul 4, 2018

Member

It is not really valid, but something like CREATE 2ABLE would have failed previously.

@bokken

bokken approved these changes Jul 4, 2018

vlsi added some commits Jun 30, 2018

@vlsi vlsi force-pushed the vlsi:command_tag_perf branch from 7c7f298 to db91492 Jul 5, 2018

@vlsi vlsi removed the needs-review label Jul 10, 2018

@vlsi vlsi merged commit da831de into pgjdbc:master Jul 10, 2018

2 checks passed

codecov/project No report found to compare against
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@seut seut referenced this pull request Nov 30, 2018

Closed

Upgrade crate-jdbc to 2.5.0 (JDK 11 compatible). #7947

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment