Skip to content
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

Prepared statement performance fixes #11

Merged

Conversation

RDearnaley
Copy link

  1. Further speedups to prepared statement hashing

  2. Caching of '?' chararacter positiobs in prepared statements to speed parameter substitution

1) Further speedups to prepared statement hashing

2) Caching of '?' chararacter positiobs in prepared statements to speed parameter substitution
Copy link
Author

@RDearnaley RDearnaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to ParsedSQLMetadata.java, SQLServerParameterMetaData.java, and SQLServerPreparedStatement.java are all for the caching of parameter positions. Of the changes to SQLServerConnection.java, most of them are or the caching of parameter positions; the exceptions are lines 126/127 (a simple speedup related to the CityHash128 change -- originally suggested by @brettwooldridge), and lines 130/131-133 (also a speedup related to the CityHash128 change). Note that line 132 of this discards the top 8 bits of the SQL text while converting it to a byte array -- the SQL will mostly be 7-bit ASCII, and in our application is entirely 7-bit ASCII, so for us this gives maximum hashing performance, but for some customers the SQL could contain UTF-16 schema/table/column names. For alphabetic languages that use only a 256 character codepage, discarding the top 8 bits should still be safe, but for ideographic languages with larger codepages discarding the top 8 bits could be discarding vital information, if they had schema/tables/columns whose names differed only in the top 8-bits. So in general, this isn't safe for all locales. Possibly solutions include: doing something slower but safe for all users, providing a setting to select between fast and safe behaviors, or choosing between fast and safe behaviors depending on some locale setting.

@codecov-io
Copy link

codecov-io commented Jun 26, 2018

Codecov Report

Merging #11 into hashKeyChanges will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##             hashKeyChanges      #11      +/-   ##
====================================================
+ Coverage             48.15%   48.18%   +0.02%     
- Complexity             2574     2581       +7     
====================================================
  Files                   113      113              
  Lines                 26724    26734      +10     
  Branches               4474     4476       +2     
====================================================
+ Hits                  12870    12882      +12     
+ Misses                11717    11708       -9     
- Partials               2137     2144       +7
Flag Coverage Δ Complexity Δ
#JDBC42 47.98% <100%> (-0.07%) 2569 <3> (+3)
#JDBC43 48.15% <100%> (+0.15%) 2580 <3> (+12) ⬆️
Impacted Files Coverage Δ Complexity Δ
...oft/sqlserver/jdbc/SQLServerParameterMetaData.java 24.14% <ø> (ø) 31 <0> (ø) ⬇️
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java 50.4% <100%> (-0.21%) 160 <0> (ø)
.../microsoft/sqlserver/jdbc/SQLServerConnection.java 46% <100%> (+0.18%) 290 <3> (+2) ⬆️
...om/microsoft/sqlserver/jdbc/ParsedSQLMetadata.java 100% <100%> (ø) 0 <0> (ø) ⬇️
...n/java/com/microsoft/sqlserver/jdbc/Parameter.java 61.84% <0%> (-0.84%) 63% <0%> (ø)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 55.25% <0%> (-0.1%) 0% <0%> (ø)
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java 33.04% <0%> (-0.08%) 244% <0%> (-1%)
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 63.36% <0%> (+0.23%) 0% <0%> (ø) ⬇️
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java 39% <0%> (+0.64%) 43% <0%> (+1%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc30eff...24cf6c5. Read the comment docs.

segments = CityHash.cityHash128(s.getBytes(), 0, s.length());
byte[] bytes = new byte[s.length()];
s.getBytes(0, s.length(), bytes, 0);
segments = CityHash.cityHash128(bytes, 0, bytes.length);
Copy link

@brettwooldridge brettwooldridge Jun 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RDearnaley I would suggest going ahead with the slightly safer:

byte[] bytes = new byte[s.length() * 2];
for (int i = 0; i < s.length(); i++) {
   final int c = s.charAt(i);
   bytes[i * 2] = (byte) c;
   bytes[(i * 2) + 1] = (byte)(c >> 8);
}
segments = CityHash.cityHash128(bytes, 0, bytes.length);

1 Receiving the charAt() into an int avoids one implicit conversion, as shift operators are not supported on char directly and the (byte) c cast also likely inflates to an int before casting.

@cheenamalhotra cheenamalhotra merged commit 85ef29c into cheenamalhotra:hashKeyChanges Jul 1, 2018
cheenamalhotra pushed a commit that referenced this pull request Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants