Codecov Report

Merging #272 into dev will increase coverage by 0.55%.
The diff coverage is 51.52%.

@@             Coverage Diff              @@
##                dev     #272      +/-   ##
============================================
+ Coverage     37.23%   37.79%   +0.55%     
- Complexity     1652     1972     +320     
============================================
  Files           102      107       +5     
  Lines         23660    28006    +4346     
  Branches       3873     4846     +973     
============================================
+ Hits           8810    10585    +1775     
- Misses        13250    15527    +2277     
- Partials       1600     1894     +294

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 60d31d8...38da794. Read the comment docs.

If the parse cache is going to be shared across multiple connections you'll need a way to evict invalid entries. Otherwise the following will break a long running application until it restarts:

1. App executes SQL
2. SQL metadata is cached with value M1
3. DDL change on database - now SQL would parse to M2
4. Usage of cached M1 metadata leads to an exception

Step 4 would repeatedly give an error with no way to succeed till the app is bounced and the cache is reset.

One solution to this would be to have entries evicted if methods that are using the cache throw an exception. An error would still happen if the cache is out of date but at least it'd only happen once per running application. Subsequent attempts to repeat the same operation would then succeed as the next attempt would get fresh metadata.

Alternatively, having the cache be at the Statement level mostly avoids this issue as you'd only run into it if the DDL change happens in the middle of repeated usage of the same Statement. As most JDBC usage involves creating Statements and then closing them when the unit of work is complete, the errant window would be much smaller.

Breaking it out into pros / cons:

• Shared Connection Cache
  • Pro: Most memory efficient
  • Pro: Highest cache hits as shared across entire pool
  • Con: Requires more complicated eviction to deal with DDL changes (evict on error?)
  • Con: Must be thread safe (not a huge deal but still a minor con)
• Per Connection Cache
  • Pro: Medium memory efficiency
  • Pro: Medium cache hit rate as there will be reuse for pooled connections and shared across Statements.
  • Pro: Eviction from connection pool provides convenient place to get rid of a stale cache (i.e. just rebuild the pool)
  • Pro: Don't have to worry about thread safety or synchronization as Connections are only meant to be used by one Thread at a time.
  • Con: Requires more complicated eviction to deal with DDL changes
  • Con: May require time based eviction to prevent stale SQL from remaining cached forever though could defer to connection pool (e.g. configure your pool to evict connections from the pool after a max age)
• Per Statement Cache
  • Pro: Simplest to implement
  • Pro: Smallest "error window" when dealing DDL changes that should lead to cache invalidations
  • Pro: Don't have to worry about thread safety or synchronization as Statements are only meant to be used by one Thread at a time.
  • Con: Least efficient memory wise as each Statement would have its own cache.
  • Con: Least efficient cache hit wise as each Statement would have its own cache.

Note that I'm not advocating one over the other ... just providing some clarity on options. If it can be disabled via connection properties and gracefully handle errant evictions, the shared cache would likely be the best for the common case where DDL changes are not happening.

Wow, @brettwooldridge & @sehrope, thanks for all of the feedback!

I just added a simple implementation of actual reuse of created server-side prepared statement handles. The prepared statment handle cache is currently controlled by the SQLServerConnection.preparedStatementHandleCacheSize_SHOULD_BE_CONNECTION_STRING_PROPERTY field, a value > 0 means cache is on :-). This cache should further improve performance.

We now have two caches:

1. SQLServerPreparedStatement.parsedSQLCache (renamed per feedback) - Multi-connection/static cache. Contains metadata from parsing the SQL text on the client-side to avoid repeated parsing.
2. SQLServerConnection.preparedStatementHandleCache - Per connection cache. Contains prepared statement handles to be re-used.

Trying to reply as best I can:

1. Curious on why not to take the dependency on the Guava LRU cache. Is this mainly about download size?
2. @brettwooldridge, I renamed preparedStatementMetadataSQLCache to parsedSQLCache per request, way clearer, thanks!
3. I don't see why the parsedSQLCache should not be used across connections (and be static), it is simply tracking results of parsed strings and does not rely on connection properties. The new prepared statement handle cache I added needs to be per connection.
4. @brettwooldridge, good point of the ensureSQLSyntax() method, will start tracking that one as well.
5. @brettwooldridge, agree on not using the SQL text as key (doing POC first). Will move to a hash in a subsequent commit.
6. @brettwooldridge, sqlCommand is the original SQL text before processing, that is what we need to build the key on. Can't find it based on the post-processed SQL string. Agree that naming of userSQL vs. sqlCommand is not clear here (userSQL in fact NOT being user-provided-SQL...)
7. @sehrope, given that the parsedSQLCache has no relationship to the server at all, all it does is track metadata related to client-side-only parsing. So it does not need to be invalidated based on what happens on the server :-) Do you think we should clarify the name further to make it clearer that it is caching related to metadata from SQL-text only parsing?

Thanks!
-Tobias

@TobiasSQL

Regarding the Guava dependency, a few things.

• Yes, one is size. That is a 2.5MB dependency for an LRU cache that can be implemented in ~100 lines of code.
• Another is that projects using the current v6 driver (or before) cannot simply drop it in, as they will be broken without that dependency.
• As far as I know, no other JDBC drivers allow external dependencies. If they do in fact require them, they shade them. Not that I am suggesting shading Guava, because of its size -- even if you only wanted the dependent graph of classes for Cache.

Regarding the static cache, I think you are correct. As a pure parsed SQL cache, I don't see a reason not to share VM-wide. I was thinking ahead, and injecting my own assumptions about implementation, assuming that the metadata cache would eventually be married with the parsed SQL cache. But that is not necessarily so, clearly.

Regarding the sqlCommand, the only place it is referenced outside of the constructor is buildExecuteMetaData():

988  private ResultSet buildExecuteMetaData() throws SQLServerException {
989     String fmtSQL = sqlCommand;
990     if (fmtSQL.indexOf(LEFT_CURLY_BRACKET) >= 0) {
991        fmtSQL = (new JDBCSyntaxTranslator()).translate(fmtSQL);
992     }
993  
994     ResultSet emptyResultSet = null;
995     try {
996        fmtSQL = replaceMarkerWithNull(fmtSQL);
      ...

But it seems the result of line 991 must be the same as userSQL in the constructor, right?

199     userSQL = translator.translate(sql);

Great to see the appearance of a metadata cache ... or the beginnings of one! 🎉

I'll need a closer reading before, so take these comments with a grain of salt...

I think the prepared statement handles can be shared across Connections, right? I believe the server-side access plans are shared across connections, so it should be possible -- assuming an access scheme keyed by the hostname/port/database like that discussed above.

I also believe that in the end, one of the biggest performance wins will come from caching the metadata returned by sp_prepare with option RETURN_METADATA along side the prepared handle. I'm not sure, but one short-circuit location would likely be in buildParamTypeDefinitions(), and another in buildExecuteMetaData().

Thx @brettwooldridge!

For Guava your point on not being a simple drop-in replacement anymore settles it for me. Will replace :-)

Execution plans are shared across connections, and even users but prepared handles are not. They are tied to a specific connection. You can see if you try to do sp_prepare on two connections that both will get handle number 1 back.

Grain of salt applied :-) WRT to the metadata from sp_prepare we actually never call sp_prepare. All of our drivers now call sp_prepexec instead (which always returns metadata). I.e. we don't do the prepare until the first execution and then we prepare and execute at the same time. Following executes call sp_execute. buildExecuteMetaData seems to be only used if you want metadata for a statement without executing (in fact this should be changed from using set fmtonly to sp_describe_first_result_set).

Good point on row 991! Like you say it should be changed to:
fmtSQL = userSQL;

Made the change :-)

Thanks!
-Tobias

@TobiasSQL Most Java ORMs call the equivalent of Connection.prepareStatement() followed by stmt.getParameterMetaData(). It was the poor performance of that pattern in jTDS that led to my implementing metadata caching back in the day. However we implement the metadata caching, it is good to keep that common pattern in mind.

@TobiasSQL Could you please open this pr to dev branch? Thank you.

@v-afrafi, changed to dev, thanks for noticing!

As an alternative to Guava's cache, how about caffeine? ( https://github.com/ben-manes/caffeine ) It is a near drop-in replacement for Guava's cache, it was written by the guy that made the original guava cache (with lessons learned), and it has a much smaller footprint in general compared to guava.

A synchronized LRU is supported by Java's LinkedHashMap as a tiny extension, and can be written in less than 10 LOC. However, it does not perform well under concurrent workloads.

Attempting to solve that led to ConcurrentLinkedHashMap. This was used heavily (Cassandra, etc), and often embedded (Groovy, Camel, etc). It is small and JDK6 based.

Guava's Cache was retrofitted with a CLHM's design and embeds many features worked out in custom decorators. This provides many best practices, such as using memoization rather than racy get-compute-put. Unfortunately its original design around soft references hindered our efforts, since we had to work within that framework rather than from a stronger foundation.

Caffeine is a JDK8 rewrite that improves on the above. It uses a modern eviction policy, offers more features, and has had very healthy adoption. However, this richness comes at the cost of a modestly sized jar. Some do shade it, but its probably not ideal for a jdbc driver.

I'd recommend embedding CLHM as it is probably the best fit.

@ben-manes & @cogman, thanks! Given concurrency is important for at least the parsed SQL text cache, CLHM sounds like a good option to embed. I will take a closer look at it once I get stuff into reasonable shape :-)

@brettwooldridge , thanks for pointing out the ORM pattern using getParameterMetaData(), makes sense. I reworked things to also handle caching this metadata.

Overall I am reworking a bunch of stuff from first prototype as it has a few issues, should hopefully be able to push some changes soon.

Cleaned up prototype and fixed a bunch of issues. Depending on feedback next POR is to replace cache impl. and start using hash for cache key.

I would suggest against using Sha-1 for the hash key. Sha-1 was designed to reduce collisions in a cryptographically safe manor, but it is slow. Faster algorithms exist which have low collision rates. Particularly, fnv3 or murmur would be preferable as high performance hashing algorithms with low collision rates.

With that said, it probably would be a better idea to use the string as the key but limit the size that will be cached, since large statements are less likely to be reused. If you are concerned about size, then you could compress the statement. But I wouldn't do that as a first step.

As an FYI the commits coming in now are fixes based on findings from our test suites that have not been GitHub:ed yet.

Got it. I'll run the benchmark later today.

@TobiasSQL Have you had a look at my metadataCaching2 branch? I replaced the SHA1 hash with CityHash128 which performed very well, with no collisions over the corpus. In a local SHA1 vs. CityHash128 benchmark, CityHash128 was ~10x faster, at roughly 160ns per hash.

The reference counting is tightened up further still.

Here are the benchmark results, Best of Three runs:

mssql-jdbc-6.1.7.jre8.preview.jar

22:39:42,654 (DBWorkload.java:614) INFO  - Workload Histograms:
Completed Transactions:
Persist/01           [11697] *******************************************************************************
Retrieve/02          [11536] ******************************************************************************
Update/03            [11774] ********************************************************************************
Delete/04            [11657] *******************************************************************************

mssql-jdbc-6.1.8-SNAPSHOT.jre8.jar (mssql-jdbc/metadataCaching branch):

22:49:37,774 (DBWorkload.java:614) INFO  - Workload Histograms:
Completed Transactions:
Persist/01           [12321] ********************************************************************************
Retrieve/02          [12120] ******************************************************************************
Update/03            [12112] ******************************************************************************
Delete/04            [12080] ******************************************************************************

mssql-jdbc-6.1.8-SNAPSHOT.jre8.jar (brettwooldridge/metadataCaching2 branch):

22:58:19,075 (DBWorkload.java:614) INFO  - Workload Histograms:
Completed Transactions:
Persist/01           [12564] *******************************************************************************
Retrieve/02          [12595] ********************************************************************************
Update/03            [12457] *******************************************************************************
Delete/04            [12579] *******************************************************************************

Previous benchmark was the oltpbenchmark jpab benchmark. This is the oltpbenchmark epinions benchmark:

mssql-jdbc-6.1.7.jre8.preview.jar

23:59:17,238 (DBWorkload.java:614) INFO  - Workload Histograms:
Completed Transactions:
GetReviewItemById/01 [33985] ***************************************
GetReviewsByUser/02  [33999] ***************************************
GetAverageRatingB... [33961] ***************************************
GetItemAverageRat... [33762] ***************************************
GetItemReviewsByT... [33722] ***************************************
UpdateUserName/06    [33919] ***************************************
UpdateItemTitle/07   [33828] ***************************************
UpdateReviewRatin... [33584] ***************************************
UpdateTrustRating/09 [68105] ********************************************************************************

mssql-jdbc-6.1.8-SNAPSHOT.jre8.jar (mssql-jdbc/metadataCaching branch):

23:50:19,993 (DBWorkload.java:614) INFO  - Workload Histograms:
Completed Transactions:
GetReviewItemById/01 [34305] ****************************************
GetReviewsByUser/02  [34392] ****************************************
GetAverageRatingB... [34216] ***************************************
GetItemAverageRat... [34175] ***************************************
GetItemReviewsByT... [34087] ***************************************
UpdateUserName/06    [34530] ****************************************
UpdateItemTitle/07   [34278] ****************************************
UpdateReviewRatin... [34270] ****************************************
UpdateTrustRating/09 [68526] ********************************************************************************

mssql-jdbc-6.1.8-SNAPSHOT.jre8.jar (brettwooldridge/metadataCaching2 branch):

23:39:41,015 (DBWorkload.java:614) INFO  - Workload Histograms:
Completed Transactions:
GetReviewItemById/01 [34670] ****************************************
GetReviewsByUser/02  [34595] ***************************************
GetAverageRatingB... [34911] ****************************************
GetItemAverageRat... [35003] ****************************************
GetItemReviewsByT... [34670] ****************************************
UpdateUserName/06    [35248] ****************************************
UpdateItemTitle/07   [34690] ****************************************
UpdateReviewRatin... [34823] ****************************************
UpdateTrustRating/09 [69330] ********************************************************************************

Left comments for your PR @brettwooldridge, sorry for the delay! I am hoping we get the current caching finalized for release next week. The week after should hopefully work great, I am thinking you can then submit a PR against the dev branch (rather than metadataCaching). We mainly need to make sure all issues found by the other test cases are resolved first.

@TobiasSQL Sure, sounds great. I'm still testing and tweaking my speedify branch. 👍 But current results are looking outstanding.

oltpbench/jpab benchmark (Hibernate) running a transaction mix of 5/75/15/5 (Persist/Retrieve/Update/Delete):

mssql-jdbc-6.1.7.jre8.preview.jar 🐢

INFO  - Workload Histograms:
Completed Transactions:
Persist/01           [ 4848] *****
Retrieve/02          [72439] ********************************************************************************
Update/03            [14412] ***************
Delete/04            [ 4844] *****

mssql-jdbc-6.1.8.jre8.preview.jar (speedify) 🏇

INFO  - Workload Histograms:
Completed Transactions:
Persist/01           [ 6364] *****
Retrieve/02          [96709] ********************************************************************************
Update/03            [19439] ****************
Delete/04            [ 6487] *****

Beautiful! @brettwooldridge, is the comparison branch based on current state of metadataCaching, dev, or master, or something else?

The comparison was with v6.1.7.

@brettwooldridge, you can now open a PR against dev for your improvements :-)

Never mind, we have some more things to go through before the merge. Expecting it next week.

@brettwooldridge, this is a great point. Agree with the implementation, makes sense with calling setCapacity to me.

I was noodling on this as well, not sure about tying it to the discard queue they seem to have at best a distant relationship ;-)

Two options come to mind:

1. Expose a separate static configuration knob for the parse cache.
2. Tie the parse cache to the statement cache size (for 5x statement cache size, potentially expose the ratio)

Looking around, additionally I think we need to provide a limit for the max statement text size to place in the parse cache to prevent accidental memory hogging. My suggestion is to start with a conservative and fairly large value (say 10k), we can later make it configurable in a following release.

Thoughts?

Given that the statement cache isn't static (which I forgot) it seems to me that only option 1 above makes sense, i.e. separate configuration knob (get/setParsedSQLCacheSize).