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

Warmup Maven Embedder to improve first-project-creation UX #7590

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

mbien
Copy link
Member

@mbien mbien commented Jul 17, 2024

performance (EmbedderFactory):

  • first embedder initialization can take a while, async-profiler showed that a big chunk of it is spent within google guice
  • this starts the warmup task early, which solves the problem, since this will happen while the user is looking at the wizard
  • the warmup task is a no-op if it was already initialized

cleanup:

  • small jdk 17 renovation

the purple bits are com.google.inject.* code:

image

@mbien mbien added performance Maven [ci] enable "build tools" tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Jul 17, 2024
@mbien mbien added this to the NB23 milestone Jul 17, 2024
performance:

 - first embedder initialization can take a while, async-profiler
   showed that a big chunk of it is spent within google guice
 - this starts the warmup task early, which solves the problem, since
   this will happen while the user is looking at the wizard
 - the warmup task is a no-op if it was already initialized

cleanup:

 - small jdk 17 renovation
@mbien
Copy link
Member Author

mbien commented Jul 17, 2024

this is offtopic but this was unexpected: new Properties(other) is no copy constructor since it stores a reference to other! Since it is a Map, a collection copy-constructor rule matched for me. Reverted that change and fixed the rule.

Copy link
Member

@neilcsmith-net neilcsmith-net left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

Possibly consider two commits in future - code update and behaviour change. Makes it easier to see the actual proposed change.

The fun of legacy collections! 😆

@mbien
Copy link
Member Author

mbien commented Jul 17, 2024

I have to work on improving my stash management

@mbien mbien merged commit 2182111 into apache:master Jul 17, 2024
33 checks passed
@sdedic
Copy link
Member

sdedic commented Jul 18, 2024

Hm, I guess originally the warmup was postponed to first project open so that it does not slow down startup of the application, but it could be possibly prior ergonomics was introduced which inhibits the whole module.

@mbien
Copy link
Member Author

mbien commented Jul 18, 2024

Hm, I guess originally the warmup was postponed to first project open so that it does not slow down startup of the application, but it could be possibly prior ergonomics was introduced which inhibits the whole module.

it is still the case. It just triggers the initialization a little bit earlier now. One wizard panel earlier to be exact.

When a project is already open, there should be no difference. With no project open there won't be any initialization still.

Technically I wouldn't call it warmup, since it is about early+concurrent vs late+blocking initialization and less about running things to throw them away again. If the initialization can run concurrently, it is often best to start it as soon you know it will be need.

one day I plan to revisit this again #5638 and check what is still needed and what can be optimized for new hardware/software realities.

quick way to debug the embedder initialization (I should have added proper logging there):

diff --git a/java/maven.embedder/src/org/netbeans/modules/maven/embedder/EmbedderFactory.java b/java/maven.embedder/src/org/netbeans/modules/maven/embedder/EmbedderFactory.java
index 752f59f..7482909 100644
--- a/java/maven.embedder/src/org/netbeans/modules/maven/embedder/EmbedderFactory.java
+++ b/java/maven.embedder/src/org/netbeans/modules/maven/embedder/EmbedderFactory.java
@@ -385,8 +385,10 @@
     }
 
     public static @NonNull MavenEmbedder getProjectEmbedder() {
+        System.out.println("<<<<<<<<<<<<<<<<get>>>>>>>>>>>>>>>>>>>");
         synchronized (PROJECT_LOCK) {
             if (project == null) {
+                System.out.println("<<<<<<<<<<<<<<<<init>>>>>>>>>>>>>>>>>>>");
                 try {
                     project = createProjectLikeEmbedder();
                 } catch (PlexusContainerException ex) {
@@ -394,6 +396,7 @@
                     throw new IllegalStateException(ex);
                 }
                 projectLoaded.set(true);
+                System.out.println("<<<<<<<<<<<<<<<<done>>>>>>>>>>>>>>>>>>>");
             }
             return project;
         }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Maven [ci] enable "build tools" tests performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants