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 | Remove Enum.values() calls to avoid unnecessary array cloning #1065

Merged
merged 2 commits into from
May 28, 2019

Conversation

ulvii
Copy link
Contributor

@ulvii ulvii commented May 22, 2019

Replacing Enum.values() calls with static arrays. Fix for #1051

@ulvii
Copy link
Contributor Author

ulvii commented May 22, 2019

Hi @cogman,

I created a PR with your suggestions. Would you mind testing the attached jars and let me know whether the fix resolves your issue?
PR1065_jars.zip

@ulvii ulvii requested review from cheenamalhotra, peterbae, lilgreenbird and rene-ye and removed request for cheenamalhotra May 22, 2019 18:02
@codecov-io
Copy link

codecov-io commented May 22, 2019

Codecov Report

Merging #1065 into dev will increase coverage by 0.08%.
The diff coverage is 74.19%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #1065      +/-   ##
============================================
+ Coverage     53.08%   53.16%   +0.08%     
- Complexity     3159     3187      +28     
============================================
  Files           119      119              
  Lines         28022    28089      +67     
  Branches       4688     4710      +22     
============================================
+ Hits          14875    14934      +59     
- Misses        10912    10918       +6     
- Partials       2235     2237       +2
Impacted Files Coverage Δ Complexity Δ
...rosoft/sqlserver/jdbc/SQLServerEncryptionType.java 68.75% <50%> (+2.08%) 5 <2> (ø) ⬇️
...rosoft/sqlserver/jdbc/InternalSpatialDatatype.java 91.66% <50%> (+0.36%) 6 <2> (ø) ⬇️
...n/java/com/microsoft/sqlserver/jdbc/DataTypes.java 80.3% <77.77%> (+0.22%) 6 <0> (ø) ⬇️
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java 47.51% <0%> (-0.87%) 111% <0%> (-1%)
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java 59.69% <0%> (-0.44%) 90% <0%> (-1%)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 61.2% <0%> (-0.3%) 0% <0%> (ø)
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 51.56% <0%> (-0.07%) 255% <0%> (-1%)
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java 44.25% <0%> (+0.07%) 328% <0%> (+1%) ⬆️
.../microsoft/sqlserver/jdbc/SQLServerConnection.java 49.75% <0%> (+0.13%) 397% <0%> (+2%) ⬆️
...rc/main/java/com/microsoft/sqlserver/jdbc/TVP.java 44.53% <0%> (+0.8%) 26% <0%> (ø) ⬇️
... and 2 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 b91c7ea...b677081. Read the comment docs.

@cheenamalhotra cheenamalhotra added this to the 7.3.1 milestone May 22, 2019
@cogman
Copy link
Contributor

cogman commented May 24, 2019

@ulvii Sorry for the delay, but it looks good to me.

I created a quick test app to push in 1000 TVP entries 1000 times. These are the results I got

7.2.2, 1.17gb of allocations
The preview, 447Mb of allocations

CPU utilization was also much lower throughout the second run.

The following file is flight recorder results taken through my test run
https://github.com/cogman/demo/blob/fasterGrouping/sqltest.zip

@ulvii ulvii changed the title Fix | Remove Enum.values() calls Performance | Remove Enum.values() calls to avoid unnecessary array cloning May 27, 2019
@ulvii ulvii merged commit 8476a46 into microsoft:dev May 28, 2019
@ulvii ulvii deleted the Issue1051 branch May 28, 2019 00:53
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.

6 participants