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

refactor: use TypeInfo getPGArrayType instead of munging type name #913

Merged
merged 3 commits into from Nov 12, 2017

Conversation

@grzm
Copy link
Contributor

@grzm grzm commented Aug 8, 2017

The purpose of the getPGArrayType method is to get the array type for
a given element type. Let the TypeInfo implementation figure out how
to do that rather than appending "[]" to the element type name in
PgPreparedStatement setArray.

@vlsi
Copy link
Member

@vlsi vlsi commented Aug 8, 2017

@grzm , Is it something that can be tested?

Loading

@grzm
Copy link
Contributor Author

@grzm grzm commented Aug 8, 2017

@vlsi There's already coverage of this functionality elsewhere in the code base. Anywhere that tests exercises PgPreparedStatement.setArray confirms that it works. One in particular is jdbc4.ArrayTest:

https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/test/java/org/postgresql/test/jdbc4/ArrayTest.java#L105

Loading

@codecov-io
Copy link

@codecov-io codecov-io commented Aug 8, 2017

Codecov Report

Merging #913 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #913      +/-   ##
============================================
+ Coverage     65.91%   65.94%   +0.03%     
- Complexity     3579     3582       +3     
============================================
  Files           167      167              
  Lines         15316    15316              
  Branches       2480     2480              
============================================
+ Hits          10095    10100       +5     
+ Misses         4044     4041       -3     
+ Partials       1177     1175       -2

Loading

@vlsi
Copy link
Member

@vlsi vlsi commented Aug 8, 2017

What I mean is: can a new test be created to test this change?

Loading

@grzm
Copy link
Contributor Author

@grzm grzm commented Aug 9, 2017

I'm all for tests supporting code changes. In this case I don't see what a test would look like other than call what is already in the test suite. What would you like to see such a test do other than call setArray as other locations already are? I would have provided one if I had thought it would meaningfully add something. I'm happy to add one if you have a suggestion as to what it would look like.

Loading

@vlsi
Copy link
Member

@vlsi vlsi commented Sep 25, 2017

@grzm , do you know if there are performance implications and/or performance improvements?

Loading

@jorsol
Copy link
Member

@jorsol jorsol commented Sep 25, 2017

The main difference I can tell, is that it additionally looks for an alias, then it use the same getPGType method:

  public int getPGArrayType(String elementTypeName) throws SQLException {
    elementTypeName = getTypeForAlias(elementTypeName);
    return getPGType(elementTypeName + "[]");
  }

Loading

@grzm
Copy link
Contributor Author

@grzm grzm commented Sep 25, 2017

I haven't run the benchmark test suite, so I really can't say. I wouldn't expect any significant change. As @jorsol points out, it does do an additional alias check, but that list is fixed and small. I'm more than happy to run them to confirm. If you could point me to documentation on how to set up and run the benchmarks, I'd appreciate it. I'm using IntelliJ if that makes a difference.

Loading

@davecramer
Copy link
Member

@davecramer davecramer commented Sep 26, 2017

@grzm there is a subproject called ubenchmark which has the benchmarks in it.

Loading

@grzm grzm force-pushed the refactor-use-pg-array-type branch from d014e56 to 435c8fc Oct 30, 2017
@grzm
Copy link
Contributor Author

@grzm grzm commented Oct 30, 2017

@davecramer Looking at the existing ubenchmark tests, I didn't see anything that would exercise this specifically, so I added some. Here's how I ran the benchmarks (from the root of the pgjdbc checkout):

mvn clean package -DskipTests && java -classpath pgjdbc/target/postgresql-42.1.5-SNAPSHOT.jar:ubenchmark/target/benchmarks.jar -Duser=grzm -Dport=5496 org.postgresql.benchmark.statement.BindArray

If there's a better way to run these, please let me know. The documentation I referred to was in a comment in the FinalizeConnection class.

mvn clean package -DskipTests && java -classpath pgjdbc/target/postgresql-42.1.5-SNAPSHOT.jar:ubenchmark/target/benchmarks.jar -Duser=grzm -Dport=5496 org.postgresql.benchmark.statement.BindArray

I'm not well-versed in reading the output of the test results, but looking at the ns/op numbers, they look comparable. Here's the output for setArray:

HEAD (9813c68)

# JMH 1.17.4 (released 285 days ago, please consider updating!)
# VM version: JDK 1.8.0_45, VM 25.45-b02
# VM invoker: /Library/Java/JavaVirtualMachines/jdk1.8.0_45.jdk/Contents/Home/jre/bin/java
# VM options: -Duser=grzm -Dport=5496
# Warmup: 5 iterations, 1 s each
# Measurement: 5 iterations, 1 s each
# Timeout: 10 min per iteration
# Threads: 1 thread, will synchronize iterations
# Benchmark mode: Average time, time/op
# Benchmark: org.postgresql.benchmark.statement.BindArray.setArray

# Run progress: 0.00% complete, ETA 00:00:20
# Fork: N/A, test runs in the host VM
# *** WARNING: Non-forked runs may silently omit JVM options, mess up profilers, disable compiler hints, etc. ***
# *** WARNING: Use non-forked runs only for debugging purposes, not for actual performance runs. ***
# Warmup Iteration   1: 1650.381 ns/op
# Warmup Iteration   2: 1316.504 ns/op
# Warmup Iteration   3: 1542.541 ns/op
# Warmup Iteration   4: 3197.746 ns/op
# Warmup Iteration   5: 966.529 ns/op
Iteration   1: 953.238 ns/op
                 ·gc.alloc.rate:               777.107 MB/sec
                 ·gc.alloc.rate.norm:          1168.000 B/op
                 ·gc.churn.PS_Eden_Space:      747.590 MB/sec
                 ·gc.churn.PS_Eden_Space.norm: 1123.636 B/op
                 ·gc.count:                    2.000 counts
                 ·gc.time:                     1.000 ms

Iteration   2: 913.015 ns/op
                 ·gc.alloc.rate:                   813.470 MB/sec
                 ·gc.alloc.rate.norm:              1168.000 B/op
                 ·gc.churn.PS_Eden_Space:          680.211 MB/sec
                 ·gc.churn.PS_Eden_Space.norm:     976.664 B/op
                 ·gc.churn.PS_Survivor_Space:      0.042 MB/sec
                 ·gc.churn.PS_Survivor_Space.norm: 0.060 B/op
                 ·gc.count:                        2.000 counts
                 ·gc.time:                         1.000 ms

Iteration   3: 860.148 ns/op
                 ·gc.alloc.rate:                   859.812 MB/sec
                 ·gc.alloc.rate.norm:              1168.000 B/op
                 ·gc.churn.PS_Eden_Space:          903.178 MB/sec
                 ·gc.churn.PS_Eden_Space.norm:     1226.910 B/op
                 ·gc.churn.PS_Survivor_Space:      0.062 MB/sec
                 ·gc.churn.PS_Survivor_Space.norm: 0.084 B/op
                 ·gc.count:                        3.000 counts
                 ·gc.time:                         2.000 ms

Iteration   4: 1143.256 ns/op
                 ·gc.alloc.rate:                   649.123 MB/sec
                 ·gc.alloc.rate.norm:              1168.000 B/op
                 ·gc.churn.PS_Eden_Space:          782.311 MB/sec
                 ·gc.churn.PS_Eden_Space.norm:     1407.654 B/op
                 ·gc.churn.PS_Survivor_Space:      0.041 MB/sec
                 ·gc.churn.PS_Survivor_Space.norm: 0.074 B/op
                 ·gc.count:                        3.000 counts
                 ·gc.time:                         1.000 ms

Iteration   5: 1131.766 ns/op
                 ·gc.alloc.rate:                   652.394 MB/sec
                 ·gc.alloc.rate.norm:              1168.001 B/op
                 ·gc.churn.PS_Eden_Space:          462.736 MB/sec
                 ·gc.churn.PS_Eden_Space.norm:     828.451 B/op
                 ·gc.churn.PS_Survivor_Space:      0.021 MB/sec
                 ·gc.churn.PS_Survivor_Space.norm: 0.037 B/op
                 ·gc.count:                        2.000 counts
                 ·gc.time:                         1.000 ms


Result "org.postgresql.benchmark.statement.BindArray.setArray":
  1000.284 ±(99.9%) 499.084 ns/op [Average]
  (min, avg, max) = (860.148, 1000.284, 1143.256), stdev = 129.611
  CI (99.9%): [501.200, 1499.369] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.alloc.rate":
  750.381 ±(99.9%) 367.956 MB/sec [Average]
  (min, avg, max) = (649.123, 750.381, 859.812), stdev = 95.557
  CI (99.9%): [382.425, 1118.337] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.alloc.rate.norm":
  1168.001 ±(99.9%) 0.001 B/op [Average]
  (min, avg, max) = (1168.000, 1168.001, 1168.001), stdev = 0.001
  CI (99.9%): [1168.000, 1168.001] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.churn.PS_Eden_Space":
  715.205 ±(99.9%) 626.399 MB/sec [Average]
  (min, avg, max) = (462.736, 715.205, 903.178), stdev = 162.674
  CI (99.9%): [88.806, 1341.604] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.churn.PS_Eden_Space.norm":
  1112.663 ±(99.9%) 859.877 B/op [Average]
  (min, avg, max) = (828.451, 1112.663, 1407.654), stdev = 223.307
  CI (99.9%): [252.786, 1972.540] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.churn.PS_Survivor_Space":
  0.033 ±(99.9%) 0.091 MB/sec [Average]
  (min, avg, max) = (≈ 0, 0.033, 0.062), stdev = 0.024
  CI (99.9%): [≈ 0, 0.124] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.churn.PS_Survivor_Space.norm":
  0.051 ±(99.9%) 0.130 B/op [Average]
  (min, avg, max) = (≈ 0, 0.051, 0.084), stdev = 0.034
  CI (99.9%): [≈ 0, 0.181] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.count":
  12.000 ±(99.9%) 0.001 counts [Sum]
  (min, avg, max) = (2.000, 2.400, 3.000), stdev = 0.548
  CI (99.9%): [12.000, 12.000] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.time":
  6.000 ±(99.9%) 0.001 ms [Sum]
  (min, avg, max) = (1.000, 1.200, 2.000), stdev = 0.447
  CI (99.9%): [6.000, 6.000] (assumes normal distribution)

With this patch:

# JMH 1.17.4 (released 285 days ago, please consider updating!)
# VM version: JDK 1.8.0_45, VM 25.45-b02
# VM invoker: /Library/Java/JavaVirtualMachines/jdk1.8.0_45.jdk/Contents/Home/jre/bin/java
# VM options: -Duser=grzm -Dport=5496
# Warmup: 5 iterations, 1 s each
# Measurement: 5 iterations, 1 s each
# Timeout: 10 min per iteration
# Threads: 1 thread, will synchronize iterations
# Benchmark mode: Average time, time/op
# Benchmark: org.postgresql.benchmark.statement.BindArray.setArray

# Run progress: 0.00% complete, ETA 00:00:20
# Fork: N/A, test runs in the host VM
# *** WARNING: Non-forked runs may silently omit JVM options, mess up profilers, disable compiler hints, etc. ***
# *** WARNING: Use non-forked runs only for debugging purposes, not for actual performance runs. ***
# Warmup Iteration   1: 1967.354 ns/op
# Warmup Iteration   2: 1257.025 ns/op
# Warmup Iteration   3: 1245.982 ns/op
# Warmup Iteration   4: 939.770 ns/op
# Warmup Iteration   5: 932.599 ns/op
Iteration   1: 942.383 ns/op
                 ·gc.alloc.rate:                   737.533 MB/sec
                 ·gc.alloc.rate.norm:              1096.000 B/op
                 ·gc.churn.PS_Eden_Space:          745.200 MB/sec
                 ·gc.churn.PS_Eden_Space.norm:     1107.394 B/op
                 ·gc.churn.PS_Survivor_Space:      0.042 MB/sec
                 ·gc.churn.PS_Survivor_Space.norm: 0.062 B/op
                 ·gc.count:                        3.000 counts
                 ·gc.time:                         2.000 ms

Iteration   2: 887.995 ns/op
                 ·gc.alloc.rate:                   784.515 MB/sec
                 ·gc.alloc.rate.norm:              1096.000 B/op
                 ·gc.churn.PS_Eden_Space:          844.316 MB/sec
                 ·gc.churn.PS_Eden_Space.norm:     1179.544 B/op
                 ·gc.churn.PS_Survivor_Space:      0.083 MB/sec
                 ·gc.churn.PS_Survivor_Space.norm: 0.116 B/op
                 ·gc.count:                        4.000 counts
                 ·gc.time:                         3.000 ms

Iteration   3: 838.363 ns/op
                 ·gc.alloc.rate:                   829.052 MB/sec
                 ·gc.alloc.rate.norm:              1096.000 B/op
                 ·gc.churn.PS_Eden_Space:          861.607 MB/sec
                 ·gc.churn.PS_Eden_Space.norm:     1139.037 B/op
                 ·gc.churn.PS_Survivor_Space:      0.041 MB/sec
                 ·gc.churn.PS_Survivor_Space.norm: 0.055 B/op
                 ·gc.count:                        5.000 counts
                 ·gc.time:                         2.000 ms

Iteration   4: 1441.615 ns/op
                 ·gc.alloc.rate:                   483.515 MB/sec
                 ·gc.alloc.rate.norm:              1096.001 B/op
                 ·gc.churn.PS_Eden_Space:          434.611 MB/sec
                 ·gc.churn.PS_Eden_Space.norm:     985.148 B/op
                 ·gc.churn.PS_Survivor_Space:      0.042 MB/sec
                 ·gc.churn.PS_Survivor_Space.norm: 0.094 B/op
                 ·gc.count:                        3.000 counts
                 ·gc.time:                         2.000 ms

Iteration   5: 1029.038 ns/op
                 ·gc.alloc.rate:                   676.309 MB/sec
                 ·gc.alloc.rate.norm:              1096.001 B/op
                 ·gc.churn.PS_Eden_Space:          717.857 MB/sec
                 ·gc.churn.PS_Eden_Space.norm:     1163.332 B/op
                 ·gc.churn.PS_Survivor_Space:      0.042 MB/sec
                 ·gc.churn.PS_Survivor_Space.norm: 0.067 B/op
                 ·gc.count:                        6.000 counts
                 ·gc.time:                         3.000 ms


Result "org.postgresql.benchmark.statement.BindArray.setArray":
  1027.879 ±(99.9%) 931.292 ns/op [Average]
  (min, avg, max) = (838.363, 1027.879, 1441.615), stdev = 241.854
  CI (99.9%): [96.586, 1959.171] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.alloc.rate":
  702.185 ±(99.9%) 518.799 MB/sec [Average]
  (min, avg, max) = (483.515, 702.185, 829.052), stdev = 134.731
  CI (99.9%): [183.385, 1220.984] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.alloc.rate.norm":
  1096.001 ±(99.9%) 0.001 B/op [Average]
  (min, avg, max) = (1096.000, 1096.001, 1096.001), stdev = 0.001
  CI (99.9%): [1096.000, 1096.001] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.churn.PS_Eden_Space":
  720.718 ±(99.9%) 660.223 MB/sec [Average]
  (min, avg, max) = (434.611, 720.718, 861.607), stdev = 171.458
  CI (99.9%): [60.495, 1380.941] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.churn.PS_Eden_Space.norm":
  1114.891 ±(99.9%) 298.264 B/op [Average]
  (min, avg, max) = (985.148, 1114.891, 1179.544), stdev = 77.458
  CI (99.9%): [816.627, 1413.155] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.churn.PS_Survivor_Space":
  0.050 ±(99.9%) 0.071 MB/sec [Average]
  (min, avg, max) = (0.041, 0.050, 0.083), stdev = 0.019
  CI (99.9%): [≈ 0, 0.121] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.churn.PS_Survivor_Space.norm":
  0.079 ±(99.9%) 0.098 B/op [Average]
  (min, avg, max) = (0.055, 0.079, 0.116), stdev = 0.026
  CI (99.9%): [≈ 0, 0.177] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.count":
  21.000 ±(99.9%) 0.001 counts [Sum]
  (min, avg, max) = (3.000, 4.200, 6.000), stdev = 1.304
  CI (99.9%): [21.000, 21.000] (assumes normal distribution)

Secondary result "org.postgresql.benchmark.statement.BindArray.setArray:·gc.time":
  12.000 ±(99.9%) 0.001 ms [Sum]
  (min, avg, max) = (2.000, 2.400, 3.000), stdev = 0.548
  CI (99.9%): [12.000, 12.000] (assumes normal distribution)

Loading

@@ -0,0 +1,89 @@
/*
* Copyright (c) 2003, PostgreSQL Global Development Group
Copy link
Member

@jorsol jorsol Oct 30, 2017

Choose a reason for hiding this comment

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

(c) 2017

Loading

import java.util.Properties;
import java.util.concurrent.TimeUnit;

@Fork(value = 0, jvmArgsPrepend = "-Xmx128m")
Copy link
Member

@jorsol jorsol Oct 30, 2017

Choose a reason for hiding this comment

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

Fork value = 1

Loading

Copy link
Contributor Author

@grzm grzm Oct 30, 2017

Choose a reason for hiding this comment

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

Thanks, @jorsol ! I copied this straight from BindBoolean. I see that some of the other tests have Fork value = 1 as well. Should BindBoolean be updated (in some other patch)?

Loading

Copy link
Member

@jorsol jorsol Oct 30, 2017

Choose a reason for hiding this comment

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

Yes, it should, and also the Measurement and Warmup should be at least 10 iterations

Loading

Copy link
Contributor Author

@grzm grzm Oct 30, 2017

Choose a reason for hiding this comment

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

I'll do that as well. Would it make sense to extract these and have them set as parameters somewhere? Should they be the same across the board?

Loading

@grzm grzm force-pushed the refactor-use-pg-array-type branch 2 times, most recently from 4e04a29 to b17b085 Oct 30, 2017
@grzm
Copy link
Contributor Author

@grzm grzm commented Oct 30, 2017

Updated benchmarks with Fork=1, and warmup and measurement set to 10. I'm just including the ns/op result summary. Happy to include the full results upon request.

HEAD

# JMH 1.17.4 (released 285 days ago, please consider updating!)
# VM version: JDK 1.8.0_45, VM 25.45-b02
# VM invoker: /Library/Java/JavaVirtualMachines/jdk1.8.0_45.jdk/Contents/Home/jre/bin/java
# VM options: -Xmx128m -Duser=grzm -Dport=5496
# Warmup: 10 iterations, 1 s each
# Measurement: 10 iterations, 1 s each
# Timeout: 10 min per iteration
# Threads: 1 thread, will synchronize iterations
# Benchmark mode: Average time, time/op
# Benchmark: org.postgresql.benchmark.statement.BindArray.setArray

# Run progress: 0.00% complete, ETA 00:00:40
# Fork: 1 of 1
# Preparing profilers: FlightRecorderProfiler 
...
Result "org.postgresql.benchmark.statement.BindArray.setArray":
  1015.380 ±(99.9%) 195.903 ns/op [Average]
  (min, avg, max) = (851.901, 1015.380, 1284.863), stdev = 129.578
  CI (99.9%): [819.477, 1211.283] (assumes normal distribution)

patched

# JMH 1.17.4 (released 285 days ago, please consider updating!)
# VM version: JDK 1.8.0_45, VM 25.45-b02
# VM invoker: /Library/Java/JavaVirtualMachines/jdk1.8.0_45.jdk/Contents/Home/jre/bin/java
# VM options: -Xmx128m -Duser=grzm -Dport=5496
# Warmup: 10 iterations, 1 s each
# Measurement: 10 iterations, 1 s each
# Timeout: 10 min per iteration
# Threads: 1 thread, will synchronize iterations
# Benchmark mode: Average time, time/op
# Benchmark: org.postgresql.benchmark.statement.BindArray.setArray

# Run progress: 0.00% complete, ETA 00:00:40
# Fork: 1 of 1
...
Result "org.postgresql.benchmark.statement.BindArray.setArray":
  1122.271 ±(99.9%) 449.082 ns/op [Average]
  (min, avg, max) = (846.299, 1122.271, 1647.941), stdev = 297.040
  CI (99.9%): [673.190, 1571.353] (assumes normal distribution)

Loading

grzm added 2 commits Oct 31, 2017
The purpose of the getPGArrayType method is to get the array type for
a given element type. Let the TypeInfo implementation figure out how
to do that rather than appending "[]" to the element type name in
PgPreparedStatement setArray.
@vlsi vlsi added this to the 42.1.5 milestone Nov 12, 2017
@vlsi vlsi merged commit 634e157 into pgjdbc:master Nov 12, 2017
1 check passed
Loading
@grzm
Copy link
Contributor Author

@grzm grzm commented Nov 13, 2017

Thanks!

Loading

@vlsi vlsi removed this from the 42.1.5 milestone Jan 8, 2018
@vlsi vlsi added this to the 42.2.0 milestone Jan 8, 2018
rhavermans added a commit to bolcom/pgjdbc that referenced this issue Jul 13, 2018
…gjdbc#913)

The purpose of the getPGArrayType method is to get the array type for
a given element type. Let the TypeInfo implementation figure out how
to do that rather than appending "[]" to the element type name in
PgPreparedStatement setArray.
rhavermans added a commit to bolcom/pgjdbc that referenced this issue Jul 13, 2018
…gjdbc#913)

The purpose of the getPGArrayType method is to get the array type for
a given element type. Let the TypeInfo implementation figure out how
to do that rather than appending "[]" to the element type name in
PgPreparedStatement setArray.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants