[KEYCLOAK-19847] - Optimizations and refactoring for better/stable startup time by pedroigor · Pull Request #8928 · keycloak/keycloak

pedroigor · 2021-11-26T00:01:12Z

Basically:

There is no change to UX but internal/implementation changes
Persisted properties are no longer within the distribution but generated as part of build steps and included in the artifacts generated by Quarkus during re-augmentation. There are several benefits here, such as avoiding breaking the server if you mess up the configuration, no need to run build if you replace the classpath in the distribution, and we can potentially make our default conf/keycloak.properties even simpler by just having there the production settings.
Properties file config source is now able to load multiple files from classpath. Properties files loaded from file system (e.g.: running the distribution) have a higher priority. This is somewhat related to initial #1 above.
Moving clustering build steps to a specific class. We should try to break a bit KeycloakProcessor so that we have steps better grouped on a per-area basis. It should also help make it easier to optimize re-augmentation as build steps are run in parallel.
Removing unnecessary code paths in CLI.
Creates a fast-path for starting the server when the start command is executed without any option. This is the main usage people should consider for better startup time. We are basically avoiding running unnecessary code related to CLI args parsing and Picocli bootstrapping. It should also simplify the Picocli related code.
Moving CLI arg processing to the CLI config source instead of having it spread everywhere.
Avoid static init on KeycloakConfigSourceProvider when looking up persisted properties before the server is starting (e.g.: still processing CLI commands/options before starting the server). This is mainly important when re-augmenting the server because re-augmentatoin runs in a different classloader.
Moving bootstrap code from lifecycle observer to close the other bootstrap methods within QuarkusKeycloakApplication.
Removing unnecessary step when running the main testsuite that was causing re-augmentation to happen everytime the server is started (e.g.: just because the user profile provider was being set).
More tests and improvements to Dist.X testsuite
Running a single/smoke tests using containers. The tests are a bit unstable and fail from time to time due to timeout. Also having issues running locally in a stable fashion.

pedroigor · 2021-12-01T13:06:29Z

quarkus/deployment/src/main/java/org/keycloak/quarkus/deployment/ClusteringBuildSteps.java

Ideally, I think we should use YAML in our configuration files. But I do not want to bring this discussion now but just make sure we avoid the cost of XML parsing at runtime.

DGuhr

some comments, but nothing which should prevent this from being merged. Nice work! 👍

DGuhr · 2021-12-01T14:58:35Z

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/DefaultFactory.java

Perhaps better call it DefaultCliFactory or e.g. DefaultPicoCliFactory? This sounds a bit too generic for me

I don't mind. The class is within the cli package though. isn't redundant to change the name as you are proposing?

Whatever works best for you.

I'd like DefaultCliFactory then, if you dont mind too much, but also not hard on this :) true, redundant, but just a bit more easy qhen searching all classes and makes the intention more clear.

DGuhr · 2021-12-01T15:00:03Z

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java

can we merge this with the surrounding if()?

DGuhr · 2021-12-01T15:00:20Z

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java

again: merge this with the surrounding if()?

DGuhr · 2021-12-01T15:04:09Z

...runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSource.java

fyi: this is not enough validation. e.g. ./kc.sh start-dev --db-password= doesstill work. But it's a good place to check for it :) I have some changes in my branch which we should merge later on, which also do some sanity checking of the actual value.

DGuhr · 2021-12-01T15:09:25Z

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/Build.java

do we really not care here when delete returns false? (so we fail to delete this file?)

No, we do not. Dealing with that only complicates code, I think.

DGuhr · 2021-12-01T15:11:12Z

.../runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/PersistedConfigSource.java

can we also call this KC_PERSISTED_PROPERTIES or sth so we don't get confused? bc. we also have keycloak.properties file :)

stianst · 2021-12-02T08:24:07Z

Minor changes, there's 42 files changed ;)

Could you please describe what the changes are in the description or JIRA?

pedroigor · 2021-12-02T12:03:48Z

In terms of startup time, I expected that we have a more stable startup time around 3.5/3.6s. We should have here an improvement of around ~200-300ms.

New:

Performance counter stats for './kc.sh -Dkc.launch.mode=test start' (10 runs):

            3.8944 +- 0.0155 seconds time elapsed  ( +-  0.40% )
            
Best:

2021-12-01 22:26:49,063 INFO  [io.quarkus] (main) Keycloak 16.0.0-SNAPSHOT on JVM (powered by Quarkus 2.4.2.Final) started in 3.538s. Listening on: http://0.0.0.0:8080                       
2021-12-01 22:26:49,215 INFO  [io.quarkus] (main) Keycloak stopped in 0.150s


Old:

Performance counter stats for './kc.sh -Dkc.launch.mode=test start' (10 runs):

            3.9884 +- 0.0237 seconds time elapsed  ( +-  0.59% )
            
Best:

2021-12-01 22:30:12,481 INFO  [io.quarkus] (main) Keycloak 16.0.0-SNAPSHOT on JVM (powered by Quarkus 2.4.2.Final) started in 3.616s. Listening on: http://0.0.0.0:8080
2021-12-01 22:30:12,626 INFO  [io.quarkus] (main) Keycloak stopped in 0.143s

At least for me, the upstream version varies a lot from 3.6 - 4.s. Note that the time above also takes into account the stop time. The configuration was set as follows:

./kc.sh build --http-enabled true --hostname-strict false --db postgres --db-username keycloak --db-password password --cluster=default --hostname-strict-https false --cluster local

No clustering. With clustering, I also got a better startup time (using default udp stack) mainly when another node is joining.

Flame graphs are pretty much the same between upstream and these changes. Except for Picocli related frames when running the fast-path (start using a pre-built server without no options).

pedroigor · 2021-12-02T12:10:45Z

@DGuhr We are really taking milliseconds here. Any idea on how to reduce even more is very appreciated.

pedroigor · 2021-12-02T12:45:06Z

@DGuhr I'm wondering if we should introduce a specific kc.test.mode == exit-no-shutdown to make the stop time more stable by removing the steps that close the session factory.

It should help to focus on startup time only. A sample result is:

Performance counter stats for './kc.sh -Dkc.launch.mode=exit-no-shutdown start' (10 runs):
            3.7790 +- 0.0278 seconds time elapsed  ( +-  0.74% )

A bit better than before and the time stable at:

2021-12-02 09:40:22,977 INFO  [io.quarkus] (main) Keycloak 16.0.0-SNAPSHOT on JVM (powered by Quarkus 2.4.2.Final) started in 3.539s. Listening on: http://0.0.0.0:8080                       
2021-12-02 09:40:23,008 INFO  [io.quarkus] (Shutdown thread) Keycloak stopped in 0.028s

Stop time keeps around 0.025-0.030s.

…artup time

pedroigor force-pushed the KEYCLOAK-19847 branch 8 times, most recently from 3d0e9b7 to 9a57ba7 Compare December 1, 2021 00:42

pedroigor commented Dec 1, 2021

View reviewed changes

DGuhr previously approved these changes Dec 1, 2021

View reviewed changes

pedroigor dismissed DGuhr’s stale review via 2ec440e December 1, 2021 15:42

pedroigor marked this pull request as ready for review December 1, 2021 15:43

pedroigor force-pushed the KEYCLOAK-19847 branch 4 times, most recently from fb16a10 to 7c97d44 Compare December 1, 2021 21:35

pedroigor force-pushed the KEYCLOAK-19847 branch from 7c97d44 to 045a0a0 Compare December 2, 2021 11:39

pedroigor changed the title ~~[KEYCLOAK-19847] - Minor changes to server startup and config~~ [KEYCLOAK-19847] - Optimizations and refactoring for better/stable startup time Dec 2, 2021

pedroigor added area/dist/quarkus kind/enhancement Categorizes a PR related to an enhancement labels Dec 2, 2021

[KEYCLOAK-19847] - Optimizations and refactoring for better/stable st…

214707e

…artup time

pedroigor force-pushed the KEYCLOAK-19847 branch from 045a0a0 to 214707e Compare December 2, 2021 15:07

DGuhr approved these changes Dec 2, 2021

View reviewed changes

stianst approved these changes Dec 2, 2021

View reviewed changes

pedroigor merged commit 9a4ab82 into keycloak:main Dec 2, 2021

Conversation

pedroigor commented Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DGuhr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stianst commented Dec 2, 2021

Uh oh!

pedroigor commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedroigor commented Dec 2, 2021

Uh oh!

pedroigor commented Dec 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pedroigor commented Nov 26, 2021 •

edited

Loading

pedroigor commented Dec 2, 2021 •

edited

Loading