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

Use -XX:ArchiveClassesAtExit for AppCDS creation in Java 17+ #29574

Merged
merged 2 commits into from Dec 1, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Nov 30, 2022

Not only does this simplify the process of generating the AppCDS,
but for some reason it also leads to an archive that is both smaller
and more useful for the JVM (meaning that it reduces startup time
more).

For example using the rest-json-quickstart on my machine I got the following:

Without AppCDS: ~450ms
With old AppCDS: ~400ms
With new AppCDS: ~340ms

Depending on how shutdown is performed, shutdownListeners
maybe null, so we need to guard against it.
Not only does this simplify the process of generating the AppCDS,
but for some reason it also leads to an archive that is both smaller
and more useful for the JVM (meaning that it reduces startup time
more)
@geoand geoand changed the title Use -XX:ArchiveClassesAtExit for AppCDS creation in Java 17 Use -XX:ArchiveClassesAtExit for AppCDS creation in Java 17+ Nov 30, 2022
@geoand
Copy link
Contributor Author

geoand commented Nov 30, 2022

@loicmathieu you'll probably appreciate this one

@loicmathieu
Copy link
Contributor

@geoand this is indead interesting! It should not make any difference, on the same Java version, to make it one way or the other, as far as I know the new option is just for simplicity sake.

I need to give it a second try as I tested it a few months ago but didn't deploy it in production because on my CI a different image is used to build the apps than the one that runs it, and sometimes there is version divergence (not the same JVM build) and AppCDS require the exact same JVM version not just the same major version.

Anyway, great PR.

Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +33 to +35
if (shutdownListeners == null) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Best to move it under the debug log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually think this one makes a little more sense

@geoand geoand merged commit 641f5a9 into quarkusio:main Dec 1, 2022
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 1, 2022
@geoand geoand deleted the appcds-exit branch December 1, 2022 06:36
@geoand
Copy link
Contributor Author

geoand commented Dec 2, 2022

@Sanne @maxandersen this makes our AppCDS story even better.

There are still things we can (like @Sanne's idea of having extensions contribute to the warmup - although in a test I did with Jackson it did not make any difference), but I feel that with this, we are at last in a pretty good place.

@Sanne
Copy link
Member

Sanne commented Dec 2, 2022

@geoand very nice!

Regarding the idea of having extensions "contribute" - how did you model that experiment, at high level?

I was expecting the extension could either trigger some operations (needs some ad-hoc knowledge to make some choices which have no durable side-effects and actually do triggers some kind of useful warmup), or list some key classed for forceful initialization. More likely a combination. Is that similar to your experiment?

@geoand
Copy link
Contributor Author

geoand commented Dec 2, 2022

It looked something like this:

@Recorder
public class AppCDSRecorder {

    public void controlGenerationAndExit() {
        if (isAppCDSRun()) {
            warmup();
            ensureShutdown();
        }
    }

    private static boolean isAppCDSRun() {
        return Boolean.parseBoolean(System.getProperty("quarkus.appcds.generate", "false"));
    }

    private static void warmup() {
        for (AdditionalWarmup additionalWarmup : ServiceLoader.load(AdditionalWarmup.class)) {
            try {
                additionalWarmup.perform();
            } catch (Exception e) {
                System.err.println("Unable to perform warmup step");
                e.printStackTrace();
            }
        }
    }

    private static void ensureShutdown() {
        CountDownLatch latch = new CountDownLatch(1);
        new Thread(new Runnable() {
            @Override
            public void run() {
                Quarkus.blockingExit();
                latch.countDown();
            }
        }).start();
        try {
            latch.await(5, TimeUnit.SECONDS);
        } catch (InterruptedException e) {
            System.err.println("Unable to properly shutdown Quarkus application when creating AppCDS");
        }
        throw new PreventFurtherStepsException();
    }
}

where AdditionalWarmup is:

public interface AdditionalWarmup {

    void perform() throws Exception;
}

The sample I used for Jackson was:

public class JacksonAdditionalWarmup implements AdditionalWarmup {

    @Override
    public void perform() throws Exception {
        Map<String, Object> json = Map.of("foo", "bar", "list", List.of(1, 2, 3));
        new ObjectMapper().writeValueAsString(json);
    }
}

It likely did not have any effect because it seems like most of the Jackson classes are now (after the addition of this PR) already part of the archive.

@loicmathieu
Copy link
Contributor

Shouldn't some of the new PreloadClassesBuildStep and it's processor be reused for this ?

It was designed for CRaC but it's more or less the same principle: having a way to preload classes as some JVM initialization class to warmup the JVM.

@geoand
Copy link
Contributor Author

geoand commented Dec 2, 2022

Yes, there are definitely possible synergies there. I just wanted to throw together something quick and dirty which is why I tried ^

@loicmathieu
Copy link
Contributor

Yeah, this can be done as a followup to share init code for CRaC, AppCDS and SnapStart

@Sanne
Copy link
Member

Sanne commented Dec 2, 2022

I like were this is going :)

N.B. Not sure if you tested, but when doing pre-loading of classes we need to also to make sure it's happening in the same classloader as the app would normally load them with.

@geoand
Copy link
Contributor Author

geoand commented Dec 2, 2022

N.B. Not sure if you tested, but when doing pre-loading of classes we need to also to make sure it's happening in the same classloader as the app would normally load them with.

Yup, I've been bitten by that one in the past when looking into AppCDs :)

@Sanne
Copy link
Member

Sanne commented Dec 2, 2022

p.s. the JVM has some flags (don't remember them on top of my head) to log class loading events as they happen. Might be useful to validate some approaches.

@loicmathieu
Copy link
Contributor

-Xlog:class,cds should display everything !

cds only will print issues like JVM mismatch for ex.

@gsmet gsmet modified the milestones: 2.16 - main, 2.15.0.Final Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants