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

Performance | Improve performance of driver by continuously cleaning up ActivityIDs stored in internal Map #1020

Merged
merged 24 commits into from
Apr 9, 2019

Conversation

peterbae
Copy link
Contributor

@peterbae peterbae commented Apr 3, 2019

Replaces PR #1018.

Every time ActivityCorrelator::cleanupActivityId is called, the driver now iterates through the Activity ID map and removes entries that are no longer needed.

To alleviate this change from affecting the driver's performance, I've made changes during the pre-login process to not create an ActivityID if Activity Trace is not on (before, the driver was creating an ActivityID during pre-login regardless of the Activity Trace being on or not), because Activity ID is an optional part of the pre-login header. This means that the Activity ID map will be empty if Activity Trace is turned off, as opposed to before where the Activity ID map was getting populated regardless.

I've also added tests to show that the Activity ID map is no longer leaking memory.

@David-Engel
Copy link
Collaborator

I like the additional change to not populate the map at all if tracing is off. Other than one optional check in my comment, looks good to me.

@codecov-io
Copy link

codecov-io commented Apr 4, 2019

Codecov Report

Merging #1020 into dev will increase coverage by 0.36%.
The diff coverage is 25.77%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #1020      +/-   ##
============================================
+ Coverage     50.13%   50.49%   +0.36%     
- Complexity     2882     2938      +56     
============================================
  Files           120      120              
  Lines         27989    27999      +10     
  Branches       4677     4685       +8     
============================================
+ Hits          14032    14139     +107     
- Misses        11699    11709      +10     
+ Partials       2258     2151     -107
Flag Coverage Δ Complexity Δ
#JDBC42 50.03% <25.77%> (+0.3%) 2897 <16> (+52) ⬆️
#JDBC43 50.46% <25.77%> (+0.43%) 2936 <16> (+62) ⬆️
Impacted Files Coverage Δ Complexity Δ
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java 47.16% <0%> (ø) 189 <0> (ø) ⬇️
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 61.19% <0%> (+0.28%) 143 <0> (+6) ⬆️
...soft/sqlserver/jdbc/SQLServerDatabaseMetaData.java 35.77% <0%> (ø) 67 <10> (ø) ⬇️
.../com/microsoft/sqlserver/jdbc/SQLServerDriver.java 83.36% <0%> (ø) 36 <0> (ø) ⬇️
...m/microsoft/sqlserver/jdbc/SQLServerException.java 72.85% <0%> (-5.25%) 32 <0> (-1)
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java 43.98% <0%> (+0.11%) 323 <0> (+3) ⬆️
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java 63.36% <100%> (+0.86%) 95 <0> (+4) ⬆️
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 57.93% <50%> (+2.37%) 0 <0> (ø) ⬇️
.../microsoft/sqlserver/jdbc/SQLServerConnection.java 47.26% <51.85%> (+0.67%) 367 <0> (+23) ⬆️
...m/microsoft/sqlserver/jdbc/ActivityCorrelator.java 78.37% <90%> (-15.91%) 9 <6> (+2)
... and 18 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 caddd83...a0e37aa. Read the comment docs.

@peterbae
Copy link
Contributor Author

peterbae commented Apr 4, 2019

Attaching the latest jars (zip file contains both JRE8/11 versions) for testing:
mssql-jdbc-7.3.0-preview-PR1020.zip

@cheenamalhotra cheenamalhotra added this to the 7.3.1 milestone Apr 8, 2019
cheenamalhotra
cheenamalhotra previously approved these changes Apr 9, 2019
rene-ye
rene-ye previously approved these changes Apr 9, 2019
lilgreenbird
lilgreenbird previously approved these changes Apr 9, 2019
@peterbae peterbae dismissed stale reviews from lilgreenbird, rene-ye, and cheenamalhotra via a0e37aa April 9, 2019 21:13
@peterbae peterbae merged commit d436203 into microsoft:dev Apr 9, 2019
@peterbae peterbae changed the title Fix ActivityID map getting not cleaned up properly Performance | Improve performance of generation and deletion of ActivityID map May 27, 2019
@peterbae peterbae changed the title Performance | Improve performance of generation and deletion of ActivityID map Performance | Improve performance of driver by continuously cleaning up ActivityIDs stored in internal Map May 27, 2019
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.

7 participants