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

Improve startup time #1773

Closed
patric-r opened this issue Aug 10, 2022 · 5 comments
Closed

Improve startup time #1773

patric-r opened this issue Aug 10, 2022 · 5 comments
Milestone

Comments

@patric-r
Copy link

patric-r commented Aug 10, 2022

For some short-living applications, every millisecond counts.
Therefore I noticed that picocli consumes most of its startup time (88%) in

picocli.CommandLine.defaultFactory():
        static Class<?> GROOVY_CLOSURE_CLASS = loadClosureClass();

In my case, this field is never accessed.
When is this the case?
Can we switch to lazy loading?

Profiler output (tracing mode):
image

@remkop
Copy link
Owner

remkop commented Aug 10, 2022

Thank you for raising this!
I’d love to improve this.

Will you be able to provide a pull request for this?

@patric-r
Copy link
Author

Hi @remkop,

I can but unfortunately not on short-term as I am blocked with other things.
Without knowing much of the inner architecture of picocli, IMHO this should be easy, just moving the early field initialization 2 lines down into the picocli.CommandLine.DefaultFactory.create(Class<T>) method and ensuring that this is done only once.

@remkop remkop closed this as completed in 373ddbf Aug 18, 2022
@remkop
Copy link
Owner

remkop commented Aug 18, 2022

@patric-r Sorry for the late response.

I don't think lazy initialization would make much difference, because the DefaultFactory is used quite a lot.
Instead, I added a way to switch off support for this feature.

From the next release, applications can improve startup time by setting system property picocli.disable.closures to true .
This means the Closure class will not be loaded.
I believe that this will meet your requirement, but let me know if you see any problem with this.

@remkop remkop added this to the 4.7 milestone Aug 18, 2022
@patric-r
Copy link
Author

patric-r commented Aug 18, 2022

@remkop Thanks for your fix!

Would you mind implementing an API alternative to the system property, as we are not in full control of system properties at our customers nor do we want to modify system properties during runtime?
(something like CommandLine.setCaseInsensitiveEnumValuesAllowed() would be desirable)

Regarding lazy-loading:
With lazy-loading I didn't mean to avoid DefaultFactory but some kind of auto-detection if the feature is required and only if it is, to load the groovy classes.
But when now looking closer at the code I don't know if it's achievable at all, as you need to load the class in order to do the GROOVY_CLOSURE_CLASS.isAssignableFrom(cls) check...

@remkop
Copy link
Owner

remkop commented Aug 18, 2022

@patric-r I have very little time to work on picocli recently.

Unless someone submits a PR with thorough tests and documentation, I don't see myself work on an API alternative for this in the near future... 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants