Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upperf: short circuit Oid.BOOL in getBoolean #745
Conversation
davecramer
reviewed
Feb 7, 2017
I'd think putting the col=columnIndex in the if would avoid creating the col object ?
This comment has been minimized.
This comment has been minimized.
codecov-io
commented
Feb 7, 2017
•
Codecov Report@@ Coverage Diff @@
## master #745 +/- ##
============================================
- Coverage 64.42% 64.42% -0.01%
- Complexity 3469 3470 +1
============================================
Files 164 164
Lines 15147 15150 +3
Branches 2501 2503 +2
============================================
+ Hits 9759 9760 +1
Misses 4156 4156
- Partials 1232 1234 +2Continue to review full report at Codecov.
|
| @@ -1945,8 +1945,12 @@ public boolean getBoolean(int columnIndex) throws SQLException { | ||
| return false; // SQL NULL | ||
| } | ||
| int col = columnIndex - 1; | ||
| if (Oid.BOOL == fields[col].getOID()) { | ||
| return 't' == (char) (this_row[col][0] & 0xFF); |
This comment has been minimized.
This comment has been minimized.
vlsi
Feb 8, 2017
Member
Does this buy much?
Can a test be added to cover that?
Is this resilient against ArrayIndexOutOfBoundsException?
This comment has been minimized.
This comment has been minimized.
panchenko
Feb 8, 2017
Index bounds are checked first in this function.
Should be faster, as it does not allocate new objects.
Just for my understanding: is it necessary to check TEXT_FORMAT/BINARY_FORMAT?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
panchenko
Feb 8, 2017
so, something like:
final byte[] value = this_row[col];
return value.length == 1 && value[0] == (byte) 't';
This comment has been minimized.
This comment has been minimized.
Preliminary test (added to the PR) with the length check as suggested: After: That is about ~95% performance increase. |
This comment has been minimized.
This comment has been minimized.
|
Just for my elucidation is the score nanoseconds, milliseconds, or ?
Dave Cramer
…On 8 February 2017 at 12:38, Jorge Solorzano ***@***.***> wrote:
Does this buy much?
Preliminary test (added to the PR) with the length check as suggested:
Before:
Benchmark (rowsize) Mode Cnt Score Error Units
ProcessBoolean.getBoolean 5 avgt 10 690.743 ± 21.417 ns/op
ProcessBoolean.getBoolean 10 avgt 10 1623.945 ± 50.107 ns/op
ProcessBoolean.getBoolean 50 avgt 10 9092.337 ± 149.268 ns/op
ProcessBoolean.getBoolean 100 avgt 10 17716.010 ± 913.568 ns/op
ProcessBoolean.getBoolean 10000 avgt 10 1788860.346 ± 14054.267 ns/op
After:
Benchmark (rowsize) Mode Cnt Score Error Units
ProcessBoolean.getBoolean 5 avgt 10 12.155 ± 0.041 ns/op
ProcessBoolean.getBoolean 10 avgt 10 20.835 ± 0.096 ns/op
ProcessBoolean.getBoolean 50 avgt 10 103.061 ± 2.483 ns/op
ProcessBoolean.getBoolean 100 avgt 10 194.745 ± 0.969 ns/op
ProcessBoolean.getBoolean 10000 avgt 10 21071.552 ± 170.534 ns/op
That is about ~95% performance increase.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#745 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAYz9ij_a93xbTFjXRMWYeiKieo6jH-Dks5raf2tgaJpZM4L6KE->
.
|
This comment has been minimized.
This comment has been minimized.
|
@davecramer it's nanoseconds |
| int col = columnIndex - 1; | ||
| if (Oid.BOOL == fields[col].getOID()) { | ||
| final byte[] value = this_row[col]; | ||
| return (value.length == 1) && ('t' == (char) (value[0] & 0xFF)); |
This comment has been minimized.
This comment has been minimized.
panchenko
Feb 9, 2017
is this "byte to char" conversion necessary?
(byte) 't' looks shorter and overall protocol is defined in bytes, not chars.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
panchenko
Feb 9, 2017
not quite the same :-) but close:
't' == (char) (value[0] & 0xFF)(byte) 't' == value[0]
This comment has been minimized.
This comment has been minimized.
jorsol
Feb 9, 2017
Contributor
Ok, let's squeeze one nanosecond more and make it shorter :-)
116 == v[0]
| public void getBoolean() throws SQLException { | ||
| rs.first(); | ||
| while (rs.next()) { | ||
| rs.getBoolean(1); |
This comment has been minimized.
This comment has been minimized.
vlsi
Feb 9, 2017
Member
Technically speaking this code is prone to dead code elimination benchmark flaw.
Is this benchmark class any different from existing https://github.com/pgjdbc/pgjdbc/blob/master/ubenchmark/src/main/java/org/postgresql/benchmark/statement/ProcessResultSet.java#L49 ?
Could you please add boolean there?
This comment has been minimized.
This comment has been minimized.
jorsol
Feb 9, 2017
Contributor
Let me check that class, I did not want to mess with ProcessResultSet but if you think that would be a better choice I will try to use it.
This comment has been minimized.
This comment has been minimized.
jorsol
Feb 9, 2017
Contributor
dead code elimination.
This comment has been minimized.
This comment has been minimized.
|
I've tested the PR a bit. I was able to activate logging via However I do not like log format much.
I've did a bit of google and a bit of benchmarks. Here's what I've got: https://bugs.openjdk.java.net/browse/JDK-6511515: poor performance of LogRecord.inferCaller depending on java.lang.Throwable.getStackTraceElement package org.postgresql.benchmark.connection;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Threads;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.infra.Blackhole;
import org.openjdk.jmh.profile.GCProfiler;
import org.openjdk.jmh.runner.Runner;
import org.openjdk.jmh.runner.RunnerException;
import org.openjdk.jmh.runner.options.Options;
import org.openjdk.jmh.runner.options.OptionsBuilder;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.LogRecord;
import java.util.logging.SimpleFormatter;
@Fork(1)
@Measurement(iterations = 5)
@Warmup(iterations = 5)
@State(Scope.Thread)
@Threads(1)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.MICROSECONDS)
public class LoggingBenchmark {
String msg = "test";
SimpleFormatter formatter = new SimpleFormatter();
@Benchmark
public LogRecord baseline() {
return new LogRecord(Level.FINE, "test");
}
@Benchmark
public LogRecord locationInfo(Blackhole bh) {
LogRecord log = new LogRecord(Level.FINE, "test");
bh.consume(log.getSourceClassName());
bh.consume(log.getSourceMethodName());
return log;
}
@Benchmark
public String format(Blackhole bh) {
LogRecord log = new LogRecord(Level.FINE, "test");
return formatter.format(log);
}
public static void main(String[] args) throws RunnerException {
Options opt = new OptionsBuilder()
.include(LoggingBenchmark.class.getSimpleName())
.addProfiler(GCProfiler.class)
.detectJvmArgs()
.build();
new Runner(opt).run();
}
}Results: What it means is it takes 36 microseconds to format a single log line with a simple "test" message. Just in case: simple JDBC query (prepare+execute+fetch) over localhost connection takes comparable time. The formatting is performed under synchronization ( I'm not saying this logging facility should always be enabled in production at DEBUG/TRACE levels, however this level of overhead might bite us (or pgjdbc users :) ). Any thoughts if we should cleanup log format before merging the PR in? |
This comment has been minimized.
This comment has been minimized.
|
Dave Cramer
On 9 February 2017 at 16:58, Vladimir Sitnikov ***@***.***> wrote:
I've tested the PR a bit. I was able to activate logging via
-DloggerLevel=DEBUG (loggerFile was inherited).
However I do not like log format much.
Here's what I get:
фев 10, 2017 12:21:50 AM org.postgresql.Driver connect
FINE: Connecting with URL: jdbc:postgresql://localhost:5432/postgres?ApplicationName=Driver Tests&loggerLevel=DEBUG&loggerFile=target/pgjdbc-tests.log
фев 10, 2017 12:21:50 AM org.postgresql.jdbc.PgConnection <init>
FINE: PostgreSQL JDBC Driver /*$mvn.project.property.parsedversion.osgiversion$*/
фев 10, 2017 12:21:50 AM org.postgresql.jdbc.PgConnection setDefaultFetchSize
FINE: setDefaultFetchSize = 0
фев 10, 2017 12:21:50 AM org.postgresql.jdbc.PgConnection setPrepareThreshold
FINE: setPrepareThreshold = 5
фев 10, 2017 12:21:50 AM org.postgresql.core.v3.ConnectionFactoryImpl openConnectionImpl
FINE: Trying to establish a protocol version 3 connection to localhost:5432
фев 10, 2017 12:21:50 AM org.postgresql.core.v3.ConnectionFactoryImpl openConnectionImpl
FINE: Receive Buffer Size is 408 300
1. two-line log records look odd to me. Baiscally the first line is
date+location, and the second line is the message. I would prefer to have
single log lines. Any thoughts here?
2. "automatic" class and method names (see the line with timestamps).
Of course they are not free, are they?
I've did a bit of google and a bit of benchmarks. Here's what I've got:
https://bugs.openjdk.java.net/browse/JDK-6511515: poor performance of
LogRecord.inferCaller depending on java.lang.Throwable.
getStackTraceElement
package org.postgresql.benchmark.connection;
import org.openjdk.jmh.annotations.Benchmark;import org.openjdk.jmh.annotations.BenchmarkMode;import org.openjdk.jmh.annotations.Fork;import org.openjdk.jmh.annotations.Measurement;import org.openjdk.jmh.annotations.Mode;import org.openjdk.jmh.annotations.OutputTimeUnit;import org.openjdk.jmh.annotations.Scope;import org.openjdk.jmh.annotations.State;import org.openjdk.jmh.annotations.Threads;import org.openjdk.jmh.annotations.Warmup;import org.openjdk.jmh.infra.Blackhole;import org.openjdk.jmh.profile.GCProfiler;import org.openjdk.jmh.runner.Runner;import org.openjdk.jmh.runner.RunnerException;import org.openjdk.jmh.runner.options.Options;import org.openjdk.jmh.runner.options.OptionsBuilder;
import java.util.concurrent.TimeUnit;import java.util.logging.Level;import java.util.logging.LogRecord;import java.util.logging.SimpleFormatter;
@fork(1)@measurement(iterations = 5)@WarmUp(iterations = 5)@State(Scope.Thread)@threads(1)@BenchmarkMode(Mode.AverageTime)@OutputTimeUnit(TimeUnit.MICROSECONDS)public class LoggingBenchmark {
String msg = "test";
SimpleFormatter formatter = new SimpleFormatter();
@benchmark
public LogRecord baseline() {
return new LogRecord(Level.FINE, "test");
}
@benchmark
public LogRecord locationInfo(Blackhole bh) {
LogRecord log = new LogRecord(Level.FINE, "test");
bh.consume(log.getSourceClassName());
bh.consume(log.getSourceMethodName());
return log;
}
@benchmark
public String format(Blackhole bh) {
LogRecord log = new LogRecord(Level.FINE, "test");
return formatter.format(log);
}
public static void main(String[] args) throws RunnerException {
Options opt = new OptionsBuilder()
.include(LoggingBenchmark.class.getSimpleName())
.addProfiler(GCProfiler.class)
.detectJvmArgs()
.build();
new Runner(opt).run();
}
}
Results:
Benchmark Mode Cnt Score Error Units
LoggingBenchmark.baseline avgt 5 0,084 ± 0,003 us/op
LoggingBenchmark.baseline:·gc.alloc.rate avgt 5 818,191 ± 30,430 MB/sec
LoggingBenchmark.locationInfo avgt 5 21,710 ± 3,459 us/op
LoggingBenchmark.locationInfo:·gc.alloc.rate avgt 5 47,141 ± 7,540 MB/sec
LoggingBenchmark.format avgt 5 36,515 ± 5,391 us/op
LoggingBenchmark.format:·gc.alloc.rate avgt 5 338,382 ± 48,791 MB/sec
What it means is it takes 36 microseconds to format a single log line with
a simple "test" message.
The main time consumer is indeed location calculation that takes like 22us.
Just in case: simple JDBC query (prepare+execute+fetch) over localhost
connection takes comparable time.
The formatting is performed under synchronization (SimpleFormatter#format
is synchronized), so I don't even bother to benchmark how it would break
in multi-threaded environment.
I'm not saying this logging facility should always be enabled in
production at DEBUG/TRACE levels, however this level of overhead might bite
us (or pgjdbc users :) ).
Any thoughts if we should cleanup log format before merging the PR in?
Yes please, if I understand this correctly we can configure the log format
using a properties file or xml file ? If we do that how does someone change
it from an external properties file ?
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#745 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAYz9vvdvogl15XuqwijSe-aS5Q3eLAoks5ra4v2gaJpZM4L6KE->
.
|
This comment has been minimized.
This comment has been minimized.
|
More benchmark After: |
jorsol
force-pushed the
jorsol:short-circuit-bool
branch
from
c2a7c9c
to
0393ade
Feb 10, 2017
vlsi
added this to the 42.0.0 milestone
Feb 12, 2017
vlsi
merged commit e69e4a1
into
pgjdbc:master
Feb 12, 2017
added a commit
to davecramer/pgjdbc
that referenced
this pull request
Sep 19, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
jorsol commentedFeb 7, 2017
The BE sends "t" or "f" for Oid.BOOL so there is no need to fall in getString which execute again checkResultSet(), decode and trim the returned String.
All of this can be avoided and get a performance benefit.