Improve live reload experience of external artifacts on dev mode #27372

fercomunello · 2022-08-19T07:07:33Z

Fixes issue #27242

This PR:

Adds new extensible file manager implementations for the Java compilation provider, the main goal is to separate the static classpath from the reloadable one, this approach is more efficient than reopen the file manager before every compilation task, as only the reloadable part will be reset.
Adds integration tests of external reloadable artifacts feature for: Java, Kotlin & Scala.

How to reproduce the issue?

Verify that the test is working as expected:
$ mvn clean install -f integration-tests/maven/ -Dit.test=DevMojoIT#testExternalReloadableArtifacts
Then, revert all the changes, run the same tests again and see that it will fails, throwing a compilation error.

The issue initially was caused by the following commit: 175665d

This PR needs a review. For knowledge: @aloubyansky

quarkus-bot · 2022-08-19T07:07:35Z

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

title should not end up with dot

This message is automatically generated by a bot.

aloubyansky · 2022-08-24T15:54:25Z

@fercomunello there is a problem with the test.
I was looking for a more optimal way to do this. Would you mind giving my branch [1] a try and see whether the compilation time difference looks better in your app? Basically, I was trying to separate the static classpath from the reloadable one. And I think there is more we could do to make it better. Thanks.

[1] https://github.com/aloubyansky/quarkus/tree/external-reloadable-modules

aloubyansky · 2022-08-24T19:54:10Z

@fercomunello according to my simple tests my branch appears to be compiling ~50ms faster. It'd be great if you could test it as well.

aloubyansky · 2022-08-24T21:01:00Z

Testing it more, the difference appears to be a bit less than ~50ms when there are reloadable external artifacts, however, it's ~35ms slower when there are no reloadable external artifacts. I'll look a bit more into this.

aloubyansky · 2022-08-31T20:34:30Z

There seem to be relevant test failures @fercomunello

fercomunello · 2022-09-01T04:29:15Z

@aloubyansky I will have a closer look to know why LambdaDevServicesContinuousTestingTestCase.java test failed. Also, I will add a integration test for Kotlin and Scala too.

The performance is a bit better now for traditional and non-traditional use-cases, take a look of the results before and after the changes. This is a custom testThatTheApplicationIsReloadedOnJavaChange test that reload 4 times, I not commited this 2 additional reload changes because it's only useful to see the performance perspective. In the first run, the live reload is 12% better and in the next run the live reload time is also a bit faster :)

aloubyansky · 2022-09-01T04:32:50Z

@fercomunello if you make it a draft, the full CI won't run. You'd need to run the full CI in your fork to know whether it's good.

aloubyansky · 2022-09-01T05:24:23Z

It could be that LambdaDevServicesContinuousTestingTestCase isn't related to your changes, although I haven't looked into that. I saw there were some dev mode test failures in the maven module, those looked related and should be probably be looked into first.

aloubyansky · 2022-09-01T05:26:10Z

Working on changes like this one, running tests in the integration-tests/maven module locally before pushing to the remote branch may save some CI time.

Add new extensible file manager implementations for the Java compilation provider, the main goal is to separate the static classpath from the reloadable one, this approach is more efficient than reopen the file manager before every compilation task, as only the reloadable part will be reset. Add integration tests of external reloadable artifacts feature for: Java, Kotlin & Scala. Fix: quarkusio#27242

fercomunello · 2022-09-03T23:18:17Z

It could be that LambdaDevServicesContinuousTestingTestCase isn't related to your changes, although I haven't looked into that. I saw there were some dev mode test failures in the maven module, those looked related and should be probably be looked into first.

This test failed because of a small typing mistake in QuarkusCompiler.java class:

// Before
if (application.getQuarkusBootstrap().getMode() != QuarkusBootstrap.Mode.TEST && module.getTest().isPresent()) { // ... }

// After
if (application.getQuarkusBootstrap().getMode() == QuarkusBootstrap.Mode.TEST && module.getTest().isPresent()) { // ... }

The other test fails helped me to think more about the file manager implementation, so I did some refactoring and I think it's even better now :)

Now, all the relevant tests passed 👍

fercomunello · 2022-09-04T00:12:23Z

core/deployment/src/main/java/io/quarkus/deployment/dev/filesystem/ReloadableFileManager.java

+public class ReloadableFileManager extends QuarkusFileManager {
+
+    private final StandardJavaFileManager reloadableFileManager;
+
+    public ReloadableFileManager(Supplier<StandardJavaFileManager> supplier, Context context) {
+        this(supplier.get(), supplier.get(), context);
+    }
+
+    protected ReloadableFileManager(StandardJavaFileManager fileManager,
+            StandardJavaFileManager reloadableFileManager, Context context) {
+        super(fileManager, context);
+        this.reloadableFileManager = reloadableFileManager;
+        try {
+            this.reloadableFileManager.setLocation(StandardLocation.CLASS_PATH, context.getReloadableClassPath());
+        } catch (IOException e) {
+            throw new RuntimeException("Cannot initialize reloadable file manager", e);
+        }
+    }


I found it more efficient to extend from ForwardingJavaFileManager than to implement the StandardJavaFileManager interface manually, the previous code of this class is on @aloubyansky branch

fercomunello · 2022-09-04T00:15:42Z

core/deployment/src/test/java/io/quarkus/deployment/dev/FileSystemWatcherTestCase.java

-import io.quarkus.deployment.dev.filewatch.FileChangeCallback;
-import io.quarkus.deployment.dev.filewatch.FileChangeEvent;
-import io.quarkus.deployment.dev.filewatch.WatchServiceFileSystemWatcher;
+import io.quarkus.deployment.dev.filesystem.watch.FileChangeCallback;
+import io.quarkus.deployment.dev.filesystem.watch.FileChangeEvent;
+import io.quarkus.deployment.dev.filesystem.watch.WatchServiceFileSystemWatcher;


Classes moved due to the addition of the io.quarkus.deployment.dev.filesystem package

fercomunello · 2022-09-04T00:17:35Z

core/deployment/src/main/java/io/quarkus/deployment/dev/filesystem/QuarkusFileManager.java

+    public void reset(Context context) {
+        try {
+            this.fileManager.setLocation(StandardLocation.CLASS_PATH, context.getClassPath());
+            this.fileManager.setLocation(StandardLocation.CLASS_OUTPUT, List.of(context.getOutputDirectory()));
+        } catch (IOException e) {
+            throw new RuntimeException("Cannot reset file manager", e);
+        }
+    }


A file manager reset means that if any internal state of the manager needs to be cleared it will be done

fercomunello · 2022-09-04T00:22:17Z

core/deployment/src/main/java/io/quarkus/deployment/dev/filesystem/ReloadableFileManager.java

+    public void reset(Context context) {
+        try {
+            this.reloadableFileManager.close();
+            this.reloadableFileManager.setLocation(StandardLocation.CLASS_PATH, context.getReloadableClassPath());
+        } catch (IOException e) {
+            throw new RuntimeException("Cannot reset reloadable file manager", e);
+        }
+        super.reset(context);
+    }


First, close and reset the reloadable file manager, then forward the static file manager reset to super class

fercomunello · 2022-09-04T00:45:17Z

...ons/scala/deployment/src/main/java/io/quarkus/scala/deployment/ScalaCompilationProvider.java

    @Override
    public void compile(Set<File> files, Context context) {
        Settings settings = new Settings();
-        context.getClasspath().stream()
-                .map(File::getAbsolutePath)
-                .forEach(f -> settings.classpath().append(f));
+        context.getClasspath().forEach(file -> settings.classpath().append(file.getAbsolutePath()));
+        context.getReloadableClasspath().forEach(file -> settings.classpath().append(file.getAbsolutePath()));


Scala compiler settings does not expose the file manager implementation as Java, the solution is to add the reloadable paths to the classpath setting.

TODO: JavaConverters is deprecated, the recommendation is to use scala.jdk.CollectionConverters. I ask for your opinion, as it may or may not be necessary to change.

//run.compile(JavaConverters.asScalaSet(fileSet).toList()); run.compile(CollectionConverters.SetHasAsScala(fileSet).asScala().toList());

fercomunello · 2022-09-04T00:46:37Z

integration-tests/kotlin/pom.xml

-        <dependency>
-            <groupId>com.google.guava</groupId>
-            <artifactId>guava</artifactId>
-            <scope>test</scope>
-            <exclusions>
-                <exclusion>
-                    <groupId>org.checkerframework</groupId>
-                    <artifactId>checker-qual</artifactId>
-                </exclusion>
-            </exclusions>
-        </dependency>


Removed Guava dependency of this module because ImmutableMap.of() can be easily replaced by Map.of()

fercomunello · 2022-09-04T00:50:33Z

core/deployment/src/main/java/io/quarkus/deployment/dev/JavaCompilationProvider.java

+        if (LOG.isEnabled(Logger.Level.ERROR) || LOG.isEnabled(Logger.Level.WARN)) {
+            collectDiagnostics(diagnosticsCollector, (level, diagnostic) -> LOG.logf(level, "%s, line %d in %s",
+                    diagnostic.getMessage(null), diagnostic.getLineNumber(),
+                    diagnostic.getSource() == null ? "[unknown source]" : diagnostic.getSource().getName()));
+        }


Collect logs only if the log level is enabled in the application

aloubyansky · 2022-09-04T19:23:09Z

Looks good @fercomunello, thanks for a great contribution!

quarkus-bot bot added area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven area/testing labels Aug 19, 2022

fercomunello changed the title ~~Fix dev mode compilation issue when change external JARs.~~ Fix dev mode compilation issue when change external JARs Aug 19, 2022

This comment has been minimized.

Sign in to view

fercomunello force-pushed the external-reloadable-artifacts branch from 00578d1 to bcb13ac Compare August 23, 2022 19:04

fercomunello changed the title ~~Fix dev mode compilation issue when change external JARs~~ [WIP] Fix dev mode compilation issue when change external JARs Aug 23, 2022

fercomunello changed the title ~~[WIP] Fix dev mode compilation issue when change external JARs~~ Fix dev mode compilation issue when change external JARs Aug 23, 2022

gsmet requested a review from aloubyansky August 23, 2022 21:52

This comment has been minimized.

Sign in to view

fercomunello force-pushed the external-reloadable-artifacts branch from bcb13ac to 4bc5258 Compare August 31, 2022 07:55

fercomunello changed the title ~~Fix dev mode compilation issue when change external JARs~~ Fix dev mode compilation issue for sources that depends on reloadable artifacts Aug 31, 2022

fercomunello changed the title ~~Fix dev mode compilation issue for sources that depends on reloadable artifacts~~ Improve dev mode live reload experience Aug 31, 2022

fercomunello force-pushed the external-reloadable-artifacts branch from 4bc5258 to ecb79ef Compare August 31, 2022 08:50

This comment has been minimized.

Sign in to view

phillip-kruger removed area/websockets area/jackson Issues related to Jackson (JSON library) area/jakarta labels Sep 1, 2022

fercomunello changed the title ~~[WIP] Improve dev mode live reload experience~~ Improve live reload experience of external modules on dev mode Sep 3, 2022

fercomunello changed the title ~~Improve live reload experience of external modules on dev mode~~ Improve live reload experience of external artifacts on dev mode Sep 3, 2022

fercomunello force-pushed the external-reloadable-artifacts branch from 34490f6 to df8e04e Compare September 3, 2022 07:20

quarkus-bot bot added area/kotlin area/scala labels Sep 3, 2022

fercomunello marked this pull request as ready for review September 3, 2022 07:24

This comment has been minimized.

Sign in to view

fercomunello force-pushed the external-reloadable-artifacts branch from df8e04e to 00aa3b1 Compare September 3, 2022 23:04

fercomunello commented Sep 4, 2022

View reviewed changes

aloubyansky approved these changes Sep 4, 2022

View reviewed changes

Quarkus Documentation automation moved this from To do to Reviewer approved Sep 4, 2022

aloubyansky merged commit c3edb14 into quarkusio:main Sep 4, 2022

Quarkus Documentation automation moved this from Reviewer approved to Done Sep 4, 2022

quarkus-bot bot added this to the 2.13 - main milestone Sep 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve live reload experience of external artifacts on dev mode #27372

Improve live reload experience of external artifacts on dev mode #27372

fercomunello commented Aug 19, 2022 •

edited

quarkus-bot bot commented Aug 19, 2022

This comment has been minimized.

This comment has been minimized.

aloubyansky commented Aug 24, 2022

aloubyansky commented Aug 24, 2022

aloubyansky commented Aug 24, 2022

aloubyansky commented Aug 31, 2022

This comment has been minimized.

fercomunello commented Sep 1, 2022

aloubyansky commented Sep 1, 2022

aloubyansky commented Sep 1, 2022

aloubyansky commented Sep 1, 2022

This comment has been minimized.

fercomunello commented Sep 3, 2022

fercomunello Sep 4, 2022

fercomunello Sep 4, 2022

fercomunello Sep 4, 2022

fercomunello Sep 4, 2022

fercomunello Sep 4, 2022

fercomunello Sep 4, 2022

fercomunello Sep 4, 2022

aloubyansky commented Sep 4, 2022

Improve live reload experience of external artifacts on dev mode #27372

Improve live reload experience of external artifacts on dev mode #27372

Conversation

fercomunello commented Aug 19, 2022 • edited

quarkus-bot bot commented Aug 19, 2022

This comment has been minimized.

This comment has been minimized.

aloubyansky commented Aug 24, 2022

aloubyansky commented Aug 24, 2022

aloubyansky commented Aug 24, 2022

aloubyansky commented Aug 31, 2022

This comment has been minimized.

fercomunello commented Sep 1, 2022

aloubyansky commented Sep 1, 2022

aloubyansky commented Sep 1, 2022

aloubyansky commented Sep 1, 2022

This comment has been minimized.

fercomunello commented Sep 3, 2022

fercomunello Sep 4, 2022

Choose a reason for hiding this comment

fercomunello Sep 4, 2022

Choose a reason for hiding this comment

fercomunello Sep 4, 2022

Choose a reason for hiding this comment

fercomunello Sep 4, 2022

Choose a reason for hiding this comment

fercomunello Sep 4, 2022

Choose a reason for hiding this comment

fercomunello Sep 4, 2022

Choose a reason for hiding this comment

fercomunello Sep 4, 2022

Choose a reason for hiding this comment

aloubyansky commented Sep 4, 2022

fercomunello commented Aug 19, 2022 •

edited