Remove extra flush events to increase performance #43363

ahus1 · 2025-10-10T13:45:15Z

Closes keycloak#43362 Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>

ahus1 · 2025-10-10T17:27:11Z

@shawkins - may I ask you to review this PR, or should I ask a different team? Thanks!

shawkins

LGTM, just added a couple of general thoughts. Ideally we can revisit the excess flushing more systematically. If I recall it was roughly doubling the time that it takes to perform an import.

shawkins · 2025-10-10T18:37:42Z

model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java

        action.setPriority(model.getPriority());
        realm.getRequiredActionProviders().add(action);
        em.persist(action);
-        em.flush();


This seems fine. In general there are a lot of flushes in RealmAdapter that seem ad hoc beyond the pattern of persist / flush / detach.

shawkins · 2025-10-10T18:49:27Z

model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java

+        Set<ClientScopeClientMappingEntity> clientScopeEntities = clientScopes.stream()
+                .filter(clientScope -> !existingClientScopes.containsKey(clientScope.getName()))
+                .filter(clientScope -> {
+                    if (clientScope.getProtocol() == null) {
+                        // set default protocol if not set. Otherwise, we will get a NullPointer
+                        clientScope.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL);
+                    }
+                    return acceptedClientProtocols.contains(clientScope.getProtocol());
+                })
+                .map(clientScope -> {
+                    ClientScopeClientMappingEntity entity = new ClientScopeClientMappingEntity();
+                    entity.setClientScopeId(clientScope.getId());
+                    entity.setClientId(client.getId());
+                    entity.setDefaultScope(defaultScope);
+                    em.persist(entity);
+                    return entity;
+                }).collect(Collectors.toSet());
+        if (!clientScopeEntities.isEmpty()) {
+            em.flush();
+            clientScopeEntities.forEach(entity -> em.detach(entity));
+        }


In situations where you have less direct control over the transitive calls, the pattern using the EntityManagers logic looks like:

Suggested change

Set<ClientScopeClientMappingEntity> clientScopeEntities = clientScopes.stream()

.filter(clientScope -> !existingClientScopes.containsKey(clientScope.getName()))

.filter(clientScope -> {

if (clientScope.getProtocol() == null) {

// set default protocol if not set. Otherwise, we will get a NullPointer

clientScope.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL);

}

return acceptedClientProtocols.contains(clientScope.getProtocol());

})

.map(clientScope -> {

ClientScopeClientMappingEntity entity = new ClientScopeClientMappingEntity();

entity.setClientScopeId(clientScope.getId());

entity.setClientId(client.getId());

entity.setDefaultScope(defaultScope);

em.persist(entity);

return entity;

}).collect(Collectors.toSet());

if (!clientScopeEntities.isEmpty()) {

em.flush();

clientScopeEntities.forEach(entity -> em.detach(entity));

}

EntityManagers.runInBatch(session, () -> {

clientScopes.stream()

.filter(clientScope -> !existingClientScopes.containsKey(clientScope.getName()))

.filter(clientScope -> {

if (clientScope.getProtocol() == null) {

// set default protocol if not set. Otherwise, we will get a NullPointer

clientScope.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL);

}

return acceptedClientProtocols.contains(clientScope.getProtocol());

})

.forEach(clientScope -> {

ClientScopeClientMappingEntity entity = new ClientScopeClientMappingEntity();

entity.setClientScopeId(clientScope.getId());

entity.setClientId(client.getId());

entity.setDefaultScope(defaultScope);

em.persist(entity);

});

}, true);

However that will generally result in 2 flushes though, rather than the single flush that is done here.

ahus1 · 2025-10-13T07:49:48Z

@shawkins - thank you for reviewing; this was just about some small improvements I found while working on something else.

shawkins · 2025-10-13T11:04:21Z

@shawkins - thank you for reviewing; this was just about some small improvements I found while working on something else.

Yes, that is clear from the issue. Just thought it was worth mentioning how broadly flush calls are being used, and another approach that also prevents the accumulation of entities in the peristence context.

ahus1 · 2025-10-14T08:22:19Z

I'll handle this another time when I have more time to dive into this.

ahus1 · 2025-10-14T14:20:27Z

@mposolda / @pedroigor / @vramik - as we've briefly discussed it in the chat today: These seem to be some small changes and the CI still passes. So I want to proceed with this changes, but don't introduce any other changes beyond that.

This is an attempt to make use of some analysis I did to test how Keycloak performs with a lot of realms, so I didn't want to waste it as the result was already there.

Please let me know if you agree, and if this PR should be merged as is. Any additional work should be tackled in a different issue and should be planned for separately.

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2025-10-14T15:33:46Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.account.AccountRestServiceTest#listApplicationsWithoutPermission

Keycloak CI - Java Distribution IT (windows-latest - temurin - 17)

org.openqa.selenium.TimeoutException: 
java.net.SocketTimeoutException: Read timed out
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Windows Server 2025', os.arch: 'amd64', os.version: '10.0', java.version: '17.0.16'
Driver info: driver.version: HtmlUnitDriver
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2025-10-14T17:40:44Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.account.AccountRestServiceTest#listApplicationsWithoutPermission

Keycloak CI - Java Distribution IT (windows-latest - temurin - 17)

org.openqa.selenium.TimeoutException: 
java.net.SocketTimeoutException: Read timed out
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Windows Server 2025', os.arch: 'amd64', os.version: '10.0', java.version: '17.0.16'
Driver info: driver.version: HtmlUnitDriver
...

Report flaky test

vramik

LGTM, thank you @ahus1 for preparing the PR

ahus1 self-assigned this Oct 10, 2025

ahus1 force-pushed the is-43362-remove-em-flush-calls branch from 48b6395 to 5eabd3e Compare October 10, 2025 13:48

Remove extra flush events to increase performance

49dbbc5

Closes keycloak#43362 Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>

ahus1 force-pushed the is-43362-remove-em-flush-calls branch from 5eabd3e to 49dbbc5 Compare October 10, 2025 14:46

ahus1 marked this pull request as ready for review October 10, 2025 17:27

ahus1 requested a review from a team as a code owner October 10, 2025 17:27

ahus1 enabled auto-merge (squash) October 10, 2025 17:27

shawkins approved these changes Oct 10, 2025

View reviewed changes

ahus1 closed this Oct 14, 2025

auto-merge was automatically disabled October 14, 2025 08:22
Pull request was closed

ahus1 reopened this Oct 14, 2025

keycloak-github-bot bot added the flaky-test label Oct 14, 2025

keycloak-github-bot bot reviewed Oct 14, 2025

View reviewed changes

vramik approved these changes Oct 14, 2025

View reviewed changes

pedroigor approved these changes Oct 15, 2025

View reviewed changes

ahus1 merged commit 02dfb4b into keycloak:main Oct 15, 2025
206 of 208 checks passed

Remove extra flush events to increase performance #43363

Remove extra flush events to increase performance #43363

Uh oh!

Conversation

ahus1 commented Oct 10, 2025

Uh oh!

ahus1 commented Oct 10, 2025

Uh oh!

shawkins left a comment

Choose a reason for hiding this comment

Uh oh!

shawkins Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

shawkins Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

ahus1 commented Oct 13, 2025

Uh oh!

shawkins commented Oct 13, 2025

Uh oh!

ahus1 commented Oct 14, 2025

Uh oh!

ahus1 commented Oct 14, 2025

Uh oh!

keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

Uh oh!

keycloak-github-bot bot commented Oct 14, 2025

Unreported flaky test detected

org.keycloak.testsuite.account.AccountRestServiceTest#listApplicationsWithoutPermission

Uh oh!

keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

Uh oh!

keycloak-github-bot bot commented Oct 14, 2025

Unreported flaky test detected

org.keycloak.testsuite.account.AccountRestServiceTest#listApplicationsWithoutPermission

Uh oh!

vramik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants