Skip to content

Mitigate performance regressions #1772

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Mitigate performance regressions #1772

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f426bf5
Prepare issue branch.
mp911de Apr 15, 2024
03ed330
Introduce caching for configured RowMapper/ResultSetExtractor.
mp911de Apr 15, 2024
656896f
Perf hacking.
mp911de Apr 16, 2024
5bb1b8b
Refine sqlName caching, add cache for toString.
mp911de Apr 18, 2024
0b98ade
Add CachedRowMapperFactory to PartTreeJdbcQuery.
mp911de Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions .../src/main/java/org/springframework/data/relational/core/mapping/DerivedSqlIdentifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,15 @@ class DerivedSqlIdentifier implements SqlIdentifier {

private final String name;
private final boolean quoted;
private volatile IdentifierProcessing sql;
private volatile String sqlName;
private final String toString;
private volatile @Nullable CachedSqlName sqlName;

DerivedSqlIdentifier(String name, boolean quoted) {

Assert.hasText(name, "A database object must have at least on name part.");
this.name = name;
this.quoted = quoted;
this.toString = quoted ? toSql(IdentifierProcessing.ANSI) : this.name;
}

@Override
Expand All @@ -62,16 +63,15 @@ public SqlIdentifier transform(UnaryOperator<String> transformationFunction) {
@Override
public String toSql(IdentifierProcessing processing) {

if (sql != processing) {
String normalized = processing.standardizeLetterCase(name);
CachedSqlName sqlName = this.sqlName;
if (sqlName == null || sqlName.processing != processing) {

String sqlName = quoted ? processing.quote(normalized) : normalized;
this.sqlName = sqlName;
this.sql = processing;
return sqlName;
String normalized = processing.standardizeLetterCase(name);
this.sqlName = sqlName = new CachedSqlName(processing, quoted ? processing.quote(normalized) : normalized);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks really weird to my old Java eyes.
And I don't even understand why you have the local variable sqlName in the first place.
If it isn't a left over from editing, we should explain it with a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

The light-weight cache is volatile. Any comparison to the field are only valid for the duration of a single instruction. Therefore, we copy the cached field value into a local variable and do our comparisons there. After we discover that we should create a new cached variant, we create CachedSqlName, assign it into the local variable and the volatile field in a single line.

return sqlName.sqlName();
}

return sqlName;
return sqlName.sqlName();
}

@Override
Expand Down Expand Up @@ -101,6 +101,9 @@ public int hashCode() {

@Override
public String toString() {
return quoted ? toSql(IdentifierProcessing.ANSI) : this.name;
return toString;
}

record CachedSqlName(IdentifierProcessing processing, String sqlName) {
}
}
21 changes: 12 additions & 9 deletions ...onal/src/main/java/org/springframework/data/relational/core/sql/DefaultSqlIdentifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,16 @@ class DefaultSqlIdentifier implements SqlIdentifier {

private final String name;
private final boolean quoted;
private volatile IdentifierProcessing sql;
private volatile String sqlName;
private final String toString;
private volatile @Nullable CachedSqlName sqlName;

DefaultSqlIdentifier(String name, boolean quoted) {

Assert.hasText(name, "A database object name must not be null or empty");

this.name = name;
this.quoted = quoted;
this.toString = quoted ? toSql(IdentifierProcessing.ANSI) : this.name;
}

@Override
Expand All @@ -61,15 +62,14 @@ public SqlIdentifier transform(UnaryOperator<String> transformationFunction) {
@Override
public String toSql(IdentifierProcessing processing) {

if (sql != processing) {
CachedSqlName sqlName = this.sqlName;
if (sqlName == null || sqlName.processing != processing) {

String sqlName = quoted ? processing.quote(name) : name;
this.sqlName = sqlName;
this.sql = processing;
return sqlName;
this.sqlName = sqlName = new CachedSqlName(processing, quoted ? processing.quote(name) : name);
return sqlName.sqlName();
}

return sqlName;
return sqlName.sqlName();
}

@Override
Expand Down Expand Up @@ -99,6 +99,9 @@ public int hashCode() {

@Override
public String toString() {
return quoted ? toSql(IdentifierProcessing.ANSI) : this.name;
return toString;
}

record CachedSqlName(IdentifierProcessing processing, String sqlName) {
}
}
Loading