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

NPM mode gives unbearable startup time. #5803

Closed
eiswind opened this issue May 29, 2019 · 42 comments · Fixed by #5932
Closed

NPM mode gives unbearable startup time. #5803

eiswind opened this issue May 29, 2019 · 42 comments · Fixed by #5932

Comments

@eiswind
Copy link

eiswind commented May 29, 2019

I converted my app to npm mode and ran mvn package to install all the deps.
So its already

Skipping `npm install`.

When I launch the app from IntelliJ startup time increased from about 10 secs to

 Started CdsApplication.Companion in 94.948 seconds (JVM running for 95.774)

This is not a big app, it's around 70 kotlin classes.

@project-bot project-bot bot added this to Inbox - needs triage in OLD Vaadin Flow ongoing work (Vaadin 10+) May 29, 2019
@manolo
Copy link
Member

manolo commented May 29, 2019

In general, npm-mode is slower to start-up because it bundles (and transpiles for es5) all the frontend stuff. But developer cycles should be practically as fast as in bower-mode. It's something we cannot do so much because we already have webpack-dev-server configuration with the minimum timings.

Think that there are many advantages in npm-mode vs bower-mode like:

  • build is the same in dev than in prod modes, unless in bower mode that you might find differences in the app
  • we get rid of webjars, having less java deps
  • get rid of bower which is obsolete
  • you can use dev-mode to develop in IE11
  • you can debug sources even though app is delivered in a bundle
  • it's pretty much easier to use 3party JS libs or components

The solution might be that in npm-dev-mode flow is able to handle requests and return non-bundled modules from the node_modules filesystem. This needs quite a bit of work in flow and was discarded in the original design although it was prototyped. The main reason of not implementing this solution is that it does not work in IE11, apart from the complexity of the implementation, not sure if it might be considered it in a future though.

@eiswind
Copy link
Author

eiswind commented May 30, 2019

I accept that, but in my case there must be something going completeley wrong.
Webpack reports 2.5 seconds but the overall startup time is 94 seconds !every! time I start the app.
Just guessing it does something before webpack even starts, but I have no clue.

for a VERY LONG time of the time it is stalled at

Starting dev-mode updaters in /home/thomas/projects/eiswind/cds-2/cds-server folder.

after that everything goes quite fast.

@Artur-
Copy link
Member

Artur- commented May 30, 2019

Are you on Windows? I saw something similar on a Windows machine and could not reproduce it on Mac.

@eiswind
Copy link
Author

eiswind commented May 30, 2019

No I am on linux, launching with IntelliJ

@pleku
Copy link
Contributor

pleku commented May 31, 2019

Apparently https://github.com/johannesh2/textfieldformatter also gives a bad startup time on Windows.

@pleku pleku moved this from Inbox - needs triage to Product backlog in OLD Vaadin Flow ongoing work (Vaadin 10+) May 31, 2019
@pleku pleku moved this from Product backlog to Iteration Backlog in OLD Vaadin Flow ongoing work (Vaadin 10+) May 31, 2019
@eiswind
Copy link
Author

eiswind commented Jun 3, 2019

Bildschirmfoto von 2019-06-03 09-24-32

It seems to spent 93 seconds in FrontendDependencies.visitClass.
I'm running on OpenJDK 11 on Linux.

@Legioth
Copy link
Member

Legioth commented Jun 4, 2019

I suspect #5825 will have an impact on this case, but I haven't tried it out.

@eiswind
Copy link
Author

eiswind commented Jun 4, 2019

This correlates with my observations. It goes totally crazy in my case :)

@Artur-
Copy link
Member

Artur- commented Jun 12, 2019

2.0.0.rc3 contains #5825 so now it should be easy to test if there are improvements

@eiswind
Copy link
Author

eiswind commented Jun 12, 2019

Any idea how I get this into vaadin-spring rc2?

@denis-anisimov
Copy link
Contributor

Looks like https://github.com/johannesh2/textfieldformatter works better with the latest fix in the snapshot even on Mac.
But anyway someone should check it on Win.

@caalador
Copy link
Contributor

For me the startup time for the textFieldFormatter stabilised around 13s with rc3 when they for rc2 were closer to 30s. (windows)

@caalador
Copy link
Contributor

To test you could just add the dependency


        <dependency>
            <groupId>com.vaadin</groupId>
            <artifactId>flow-server</artifactId>
            <version>2.0.0.rc3</version>
        </dependency>

@eiswind
Copy link
Author

eiswind commented Jun 12, 2019

It's not compatible, so sadly I cannot test it (yet)

An attempt was made to call a method that does not exist. The attempt was made from the following location:    com.vaadin.flow.spring.VaadinServletContextInitializer.onStartup(VaadinServletContextInitializer.java:346)
The following method did not exist:
com.vaadin.flow.server.webcomponent.WebComponentConfigurationRegistry.getInstance(Ljavax/servlet/ServletContext;)Lcom/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry;

@caalador
Copy link
Contributor

Right also the spring dependency should be added:


        <dependency>
            <groupId>com.vaadin</groupId>
            <artifactId>flow-server</artifactId>
            <version>2.0.0.rc3</version>
        </dependency>
        <dependency>
            <groupId>com.vaadin</groupId>
            <artifactId>vaadin-spring</artifactId>
            <version>12.0.0.rc3</version>
        </dependency>

@Artur-
Copy link
Member

Artur- commented Jun 12, 2019

The easy way to test would be to wait for 14.0.0.beta3 (tomorrow?)

Otherwise do not override individual Flow dependencies, add Flow bom instead

            <dependency>
                <groupId>com.vaadin</groupId>
                <artifactId>flow-bom</artifactId>
                <type>pom</type>
                <scope>import</scope>
                <version>${flow.version}</version>
            </dependency>

@eiswind
Copy link
Author

eiswind commented Jun 12, 2019

I don't get it. now it is starting in bower mode, although package.json and node_modules exist from the maven build. Guess I wait for beta3

@Artur-
Copy link
Member

Artur- commented Jun 12, 2019

RIght, you need to use flow-maven-plugin 2.0.0.rc3 also instead of vaadin-maven-plugin 14.0.0.beta2

@silvan-lincan
Copy link

silvan-lincan commented Jun 16, 2019

Same issue here, with Vaadin 14.0.0.beta3 and a somewhat larger project, I get ~ 2 minutes Jetty startup on the following step:
[main] INFO dev-updater - Scanning classes to find frontend configurations and dependencies...

Is there a way to skip this step and jump directly to next step on starting webpack, on subsequent Jetty restarts, as most of the time, I don't touch the layout resources (also, no new JSModule annotations) once the project is setup and working?
Thank you.

@manolo
Copy link
Member

manolo commented Jun 17, 2019

@silvan-lincan or @eiswind we would like to setup a project able to reproduce the same issue with startup times, right now the starters we are using reports a quite acceptable startup.
Do you mind to share your setup via a simple project? and also share your dependency tree?

@silvan-lincan
Copy link

Hi @manolo
Thank you very much for your answer.
I will try to setup a project, to be as close as possible to our internal one (unfortunately, as this is proprietary, I cannot expose everything) and will let you know as soon as I have it and about the results.
This is indeed kind of important as it somehow disturbs the development flow, and with the same project and Vaadin versions between 10 and 13 we did not have this issue.
Thank you.

@eiswind
Copy link
Author

eiswind commented Jun 17, 2019

I can try to do the same on the coming weekend.

@silvan-lincan
Copy link

Hi @manolo ,
I had some time to debug a little bit, to see what is going on, and discovered the following, maybe this helps further:
In com.vaadin.flow.server.frontend.FrontendDependencies#isVisitable I think there are still classes which are not blacklisted in that regexp.
For example, everything which starts with: javax., net.sf, net.bytebuddy, org.joda, org.hibernate, org.glassfish, com.lowagie, com.google, org.hsqldb, com.fasterxml

Because of this, if we use these in our project, then, in the following method, all these classes are visited:
`com.vaadin.flow.server.frontend.FrontendDependencies#visitClass((String,EndPointData), which takes a very long time.
I noticed this during a debug session

It is a little bit complicated for me to separate things in the project so that I can make an example. But in the meantime, maybe this will help.

Thank you.

@eiswind
Copy link
Author

eiswind commented Jun 17, 2019 via email

@silvan-lincan
Copy link

Hi again @manolo , sorry to insist with this, but for me is very important as this has an impact on development time.
I managed to patch the flow-server, more precisely the regexp which is used here:

com.vaadin.flow.server.frontend.FrontendDependencies#isVisitable

and added the patterns that I mentioned in my previous comment.

^(java|sun|elemental|javax|org.(apache|atmosphere|jsoup|jboss|w3c|spring|joda|hibernate|glassfish|hsqldb)|com.(helger|spring|gwt|lowagie|google|fasterxml)|net.(sf|bytebuddy)).*|

I have now a steady startup of 40 seconds from a previous 2-4 minutes.

Is there a chance that this is taken in consideration and rolled-out?
Also, as I find this pretty sensitive, can this be exposed somehow to the user as configuration, since each new library added to the project can increase the startup of the application server at dev-time.

Thank you.

@silvan-lincan
Copy link

Could you provide a secure upload link? I would be willing to share my project.

-------- Original-Nachricht -------- An 17. Juni 2019, 15:55, Silvan Eugen Lincan schrieb:
Hi @.***(https://github.com/manolo) , I had some time to debug a little bit, to see what is going on, and discovered the following, maybe this helps further: In com.vaadin.flow.server.frontend.FrontendDependencies#isVisitable I think there are still classes which are not blacklisted in that regexp. For example, everything which starts with: javax., net.sf, net.bytebuddy, org.joda, org.hibernate, org.glassfish, com.lowagie, com.google, org.hsqldb, com.fasterxml Because of this, if we use these in our project, then, in the following method, all these classes are visited: `com.vaadin.flow.server.frontend.FrontendDependencies#visitClass((String,EndPointData), which takes a very long time. I noticed this during a debug session It is a little bit complicated for me to separate things in the project so that I can make an example. But in the meantime, maybe this will help. Thank you. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Hi @eiswind . No need for this as for me the issue is kind of clear. Please see my new recent comment.
Thank you.

@pleku
Copy link
Contributor

pleku commented Jun 17, 2019

@silvan-lincan thank you for your time and effort, really appreciate it.

I agree with you on both accounts, we need to include more common packages to the regex but it is also a good idea to make it possible for the project to blacklist more packages. Mayhaps also to whitelist.

However, the improved 40 seconds startup is also quite much, so I would like to know

  1. how much was it for this app with Vaadin 13 ?
  2. can you estimate how much of that 40 seconds is related to Vaadin based on log messages ?

@silvan-lincan
Copy link

Hi @pleku
Thank you very much for answering.

With Vaadin 13.x:
mvn jetty:run : a total of 15 seconds

With Vaadin 14.0.0.beta3:

mvn jetty:run from command line: a total of 32 seconds from which 22 Vaadin
From this 22, the most time is spend on dev-updater on scanning for frontend resources and the rest on webpack-dev.

jetty:run from Eclipse: a total of 40 seconds from which 30sec Vaadin
From this 30 sec, the proportion is the same as when running from command line.

I hope this helps further and I thank you again for replying, this means a lot.

@Legioth
Copy link
Member

Legioth commented Jun 18, 2019

Whitelisting or blacklisting will always be problematic. What if we e.g. blacklist net.bytebuddy and then the author of that library wants to use Vaadin for something? What this means in practice is that any whitelist or blacklist should be fully configurable by the developer and only used as a last resort in special cases when the built-in defaults are not sufficient.

This in turn means that we should put effort into making the defaults perform better so that special overrides are only needed in special cases. One potentially impactful approach there would be to more aggressively cache the scanning results. For caching to work in this case, the cache would have to be persisted in the developer's file system so that it's available between runs. An easy but slightly limited approach would be to put the cache somewhere in the project's /target/ directory. A more complex but also more persistent approach would be to put the cache in e.g. the user's home directory.

Another question is exactly what is cached. My gut feeling is that it would get us quite far to simply cache which .jar files are known to not contain anything of interest for our purposes. The cache could identify the files either based on (name + size + timestamp) or based on a hash of the file contents. Using a hash of file contents would also make it safe and easy to include a precomputed hash based on known transitive dependencies so that those would not have to be recomputed by every user.

@manolo
Copy link
Member

manolo commented Jun 18, 2019

Hi, @silvan-lincan, I have been able to reproduce the scan time performance problem in one of our integration tests, probably not exactly the same dependency tree than yours, but still significant to isolate the problem and able to reduce scan time. I could measure an improvement of the start time from 116889ms to 7572ms with the patch already commented here #5825, but in the branch I'm working on, I have reduced to 1350ms I'll point to that patch when it's ready so as you can check with your project

@silvan-lincan
Copy link

Hi @manolo, thank you very much. That is really great news.
Please give me a sign when you have the patch so that I can test.

@manolo
Copy link
Member

manolo commented Jun 18, 2019

@silvan-lincan can you checkout the branch in PR #5932, and do measurements with your project ?

@silvan-lincan
Copy link

Hi @manolo
I finally managed to checkout and test PR #5932.
I have now a startup of 18 seconds !
And, if I apply also the proposed blacklist enhancement, I obtain a startup of 13 seconds.
The only problem is that now I have JavaScript errors right from the beggining and could not visit the page to see if the project still works.
Can this be related to the patch, as I didn't touched the project's code?

Thank you.

@manolo
Copy link
Member

manolo commented Jun 18, 2019

It might break, although in the projects I tested the patch I didn't found any problem. You would need to check whether the target/frontend/generated-flow-imports.js remains the same compiling the project with and without the patch, also check the file with the computed dependencies at target/frontend/package.json

@silvan-lincan
Copy link

Hi @manolo
Thank you for suggestions.
I made a complete cleanup, checked again the maven dependencies so that it points to newly generated artifacts from PR #5932, rebuild and now I have the application running without issues and jetty starts fast as mentioned on my previous comment.
Thank you again and I hope that you will reach an agreement in regards with this patch, so that it finds its way on the next release.

@manolo
Copy link
Member

manolo commented Jun 18, 2019

Thanks for the feedback, there are many numbers in the thread, what is the exact improvement in ms applying the patch?

@pleku
Copy link
Contributor

pleku commented Jun 18, 2019

Thank you again and I hope that you will reach an agreement in regards with this patch, so that it finds its way on the next release.

While this won't be in Flow 2.0.0, we are going to make 2.0.1 soon hopefully with the improvements included, we just want to take time to thoroughly test it. And then it will end up into a Vaadin 14 release soon.

@silvan-lincan
Copy link

Hi @manolo
So, applying your patch I have now the following results:

Total start time: [INFO] Started @15849ms

From which the time spent on scanning: 2923ms

2019-06-18 14:02:42:689 +0200 [main] INFO com.vaadin.flow.server.startup.DevModeInitializer - Starting dev-mode updaters in
2019-06-18 14:02:42:698 +0200 [main] INFO dev-updater - Scanning classes to find frontend configurations and dependencies...
2019-06-18 14:02:45:622 +0200 [main] INFO dev-updater - took 2923ms.

Time spent on webpack: 4000ms

Hope this helps.

@silvan-lincan
Copy link

Thank you again and I hope that you will reach an agreement in regards with this patch, so that it finds its way on the next release.

While this won't be in Flow 2.0.0, we are going to make 2.0.1 soon hopefully with the improvements included, we just want to take time to thoroughly test it. And then it will end up into a Vaadin 14 release soon.

Hi @pleku
Thank you very much.

@silvan-lincan
Copy link

silvan-lincan commented Jun 18, 2019

... so with the patch, I kind of obtain the same startup speed as with Vaadin 13.x, which is super Ok.

@manolo
Copy link
Member

manolo commented Jun 18, 2019

Just to clarify 2.0.0 already includes #5933 which includes the regular expression for blacklist you sent, that should cover partially your problems. For the other we need more rework and tests.

@silvan-lincan
Copy link

Just to clarify 2.0.0 already includes #5933 which includes the regular expression for blacklist you sent, that should cover partially your problems. For the other we need more rework and tests.

Great, thank you.

@pleku pleku moved this from Iteration Backlog to In progress in OLD Vaadin Flow ongoing work (Vaadin 10+) Jun 18, 2019
@manolo manolo moved this from In progress to Review in progress in OLD Vaadin Flow ongoing work (Vaadin 10+) Jun 20, 2019
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Review in progress to Done - pending release Jun 21, 2019
@denis-anisimov denis-anisimov added this to the 2.0.1 milestone Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Development

Successfully merging a pull request may close this issue.

8 participants