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

perf: avoid BaseQueryKey.toString in CachedQuery.getSize #1227

Merged
merged 4 commits into from Jun 29, 2018
Merged
Diff settings

Always

Just for now

@@ -5,13 +5,15 @@

package org.postgresql.core;

import org.postgresql.util.CanEstimateSize;

/**
* This class is used as a cache key for simple statements that have no "returning columns".
* Prepared statements that have no returning columns use just {@code String sql} as a key.
* Simple and Prepared statements that have returning columns use {@link QueryWithReturningColumnsKey}
* as a cache key.
*/
class BaseQueryKey {
class BaseQueryKey implements CanEstimateSize {
public final String sql;
public final boolean isParameterized;
public final boolean escapeProcessing;
@@ -31,6 +33,14 @@ public String toString() {
+ '}';
}

@Override
public long getSize() {
if (sql == null) { // just in case
return 16;
}
return 16 + sql.length() * 2L; // 2 bytes per char, revise with Java 9's compact strings
}

@Override
public boolean equals(Object o) {
if (this == o) {
@@ -13,9 +13,7 @@
*/
public class CachedQuery implements CanEstimateSize {
/**
* Cache key. {@link String} or {@code org.postgresql.jdbc.CallableQueryKey}. It is assumed that
* {@code String.valueOf(key)*2} would give reasonable estimate of the number of retained bytes by
* given key (see {@link #getSize}).
* Cache key. {@link String} or {@code org.postgresql.util.CanEstimateSize}.
*/
public final Object key;
public final Query query;
@@ -25,6 +23,9 @@
private int executeCount;

public CachedQuery(Object key, Query query, boolean isFunction, boolean outParmBeforeFunc) {
assert key instanceof String || key instanceof CanEstimateSize
: "CachedQuery.key should either be String or implement CanEstimateSize."
+ " Actual class is " + key.getClass();
this.key = key;
this.query = query;
this.isFunction = isFunction;
@@ -55,7 +56,12 @@ public int getExecuteCount() {

@Override
public long getSize() {

This comment has been minimized.

Copy link
@bokken

bokken Jun 27, 2018

Member

how frequently is this method called?
would it make more sense to calculate this value once (in constructor) and simply return that calculated value each time?

This comment has been minimized.

Copy link
@vlsi

vlsi Jun 27, 2018

Author Member

Twice in the lifetime of a query: once when adding the query to the cache, then once when removing it.
Note: this method is trivial and I see no reason to cache its results.

The one with "returning columns" might indeed take some time, however the number of binds is limited with 1<<16 or so, so it should still be "good enough".

This comment has been minimized.

Copy link
@benbenw

benbenw Jun 27, 2018

Contributor

I'm not an expert of the code base but are you sure it's twice in the life time ?
It seems to be twice per execution of the cached query (borrow / release) that's why it generates so many allocations.

This comment has been minimized.

Copy link
@bokken

bokken Jun 27, 2018

Member

So that means 2x the number of times the "CachedQuery" is used over the life of the connection?

It was the "returning columns" bit that caused me some concern. Is this degree of exactness required? Would it be close enough to say columnNames.length * 32 (i.e. assume 16 chars on average)?

This comment has been minimized.

Copy link
@vlsi

vlsi Jun 27, 2018

Author Member

That's right. I stand corrected

This comment has been minimized.

Copy link
@vlsi

vlsi Jun 27, 2018

Author Member

Technically speaking, Arrays.equals would have to be called, so iterating over columnNames one more time would not hurt much.

What do you think of just adding a field for size cache for QueryWithReturningColumnsKey?

This comment has been minimized.

Copy link
@bokken

bokken Jun 27, 2018

Member

What do you think of just adding a field for size cache for QueryWithReturningColumnsKey?

And perhaps only for the portion representing the column names (i.e. what is unique to the class).

int queryLength = String.valueOf(key).length() * 2 /* 2 bytes per char */;
long queryLength;
if (key instanceof String) {
queryLength = ((String) key).length() * 2L; // 2 bytes per char, revise with Java 9's compact strings
} else {
queryLength = ((CanEstimateSize) key).getSize();
}
return queryLength * 2 /* original query and native sql */
+ 100L /* entry in hash map, CachedQuery wrapper, etc */;
}
@@ -27,6 +27,18 @@
this.columnNames = columnNames;
}

@Override
public long getSize() {
long size = super.getSize();
if (columnNames != null) {
size += 16L; // array itself
for (String columnName: columnNames) {
size += columnName.length() * 2L; // 2 bytes per char, revise with Java 9's compact strings
}
}
return size;
}

@Override
public String toString() {
return "QueryWithReturningColumnsKey{"
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.