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

When CommandLine is initialized with IFactory all subcommands get created instead of only the one called #690

Closed
danielBreitlauch opened this issue May 10, 2019 · 15 comments

Comments

@danielBreitlauch
Copy link

I create a CommandLine with an IFactory containing a Guice injector.
The cli consists of a lot of subcommands that are heavy to initialize.
All of them injection quite some heavy client instances. So I was expecting that picocli is only creating objects of subcommands that are needed. Initializing all the unused, not called subcommands generates quite some start up overhead.

@remkop
Copy link
Owner

remkop commented May 12, 2019

Sorry for my late response. I was running a marathon. :-)

I need to think about this a bit. I'm open to this in principle, as long as it does not introduce too many complexities.

@remkop remkop added this to the 4.0 milestone May 12, 2019
@danielBreitlauch
Copy link
Author

No worries (besides that you will have to work on your marathon time ;)

I dont know enough about the internals but it could be quite simple.
Do you need (sub)command class instances in order to parse the input?
The annotations should be readable from the class not the instance right?

Then the command line can be parsed without instantiating any objects.
And then only the command objects need to be instantiated that are actually used.

If I get some time I will have a look but I guess you can do that way faster ;D
Thx for considering it. I think it wil also give a nice speed boost in some instances.

@remkop
Copy link
Owner

remkop commented May 13, 2019

Not sure when I will be able to get to it, given the todo list for 4.0-beta and 4.0.

If you have time to look at it that would be great.
The place to start is probably CommandLine.Model.CommandReflection#extractCommandSpec; it currently instantiates the user object for each command and uses it to create a IScope object for the annotated fields and methods. One idea would be to create a lazy IScope implementation that does not create the instance until the get method is called.

@remkop remkop modified the milestones: 4.0, 4.1 May 16, 2019
@remkop
Copy link
Owner

remkop commented May 16, 2019

FYI: I have started to reduce the scope for picocli 4.0 in order to get something out before the end of the summer. I won't be able to get to this item until after that.
If you need something sooner, please consider contributing a pull request. :-)

@remkop
Copy link
Owner

remkop commented Jan 21, 2020

@danielBreitlauch a first cut of lazily instantiated user objects has landed in master. Please try it out and let me know what you think.

@danielBreitlauch
Copy link
Author

Thanks I will

@remkop
Copy link
Owner

remkop commented Jan 22, 2020

@danielBreitlauch Sorry, it looks like there is still more work to be done.
Commands objects are still initialized to capture the initial value of @Option and @Parameter-annotated fields; this initial value is used to clear field values before parsing in case a CommandLine instance is being reused. I need to look at this more.

remkop added a commit that referenced this issue Jan 23, 2020
@remkop
Copy link
Owner

remkop commented Jan 23, 2020

@danielBreitlauch I believe it is all fixed now.
Would be great if you get a chance to verify this.

@remkop remkop closed this as completed Jan 23, 2020
remkop added a commit that referenced this issue Jan 26, 2020
remkop added a commit that referenced this issue Jan 26, 2020
@remkop
Copy link
Owner

remkop commented Feb 12, 2020

@danielBreitlauch, All,

picocli 4.2.0 has been released, including this fix. Enjoy!

@Immueggpain
Copy link

@remkop Hi, it looks like using the HelpCommand still initializes Commands objects.

@remkop
Copy link
Owner

remkop commented Mar 1, 2020

@Immueggpain What is the problem you are experiencing? Can you tell me how to reproduce it? What did you expect to see and what do you actually see?

@remkop
Copy link
Owner

remkop commented Mar 1, 2020

@Immueggpain Do you mean a command annotated with helpCommand = true? (https://picocli.info/#_custom_help_subcommands)

@remkop remkop reopened this Mar 1, 2020
@remkop remkop closed this as completed Mar 1, 2020
@Immueggpain
Copy link

@remkop Sorry for the confusion. Here is the code:

@Command(description = "play picocli", name = "playpico", mixinStandardHelpOptions = true,
		subcommands = { HelpCommand.class, Foo.class })
public class Launcher implements Callable<Void> {

	public static void main(String[] args) {
		int exitCode = new CommandLine(new Launcher()).setCaseInsensitiveEnumValuesAllowed(true)
				.setUsageHelpLongOptionsMaxWidth(40).setUsageHelpAutoWidth(true).execute(args);
		System.exit(exitCode);
	}

	@Override
	public Void call() throws Exception {
		CommandLine.usage(this, System.out);
		return null;
	}

}

help subcommand will trigger foo's constructor

@remkop
Copy link
Owner

remkop commented Mar 1, 2020

@Immueggpain Thanks for the clarification.

The problem is not the presence of the HelpCommand subcommand.
The reason why the subcommand is instantiated is that we are creating the usage help message.
You can test this by commenting out the HelpCommand subcommand, and creating a Foo subcommand that throws an error from its constructor:

@Command(name = "foo")
static class Foo {
    public Foo() {
        throw new IllegalStateException("Don't instantiate me!");
    }
}

@Command(name = "playpico",
        description = "play picocli", mixinStandardHelpOptions = true,
        subcommands = { //CommandLine.HelpCommand.class,
                Foo.class })
static class Launcher implements Runnable {

    public static void main(String[] args) {
        int exitCode = new CommandLine(new Launcher())
                .setCaseInsensitiveEnumValuesAllowed(true)
                .setUsageHelpLongOptionsMaxWidth(40)
                .setUsageHelpAutoWidth(true)
                .execute(args);
        //System.exit(exitCode);
    }
    public void run() {
        CommandLine.usage(this, System.out);
    }
}

This will produce the following stack trace:

picocli.CommandLine$InitializationException: Could not instantiate class picocli.SubcommandTests$Foo: java.lang.reflect.InvocationTargetException
	at picocli.CommandLine$DefaultFactory.create(CommandLine.java:4948)
	at picocli.CommandLine$Model$CommandUserObject.getInstance(CommandLine.java:10364)
	at picocli.CommandLine$Model$CommandSpec.userObject(CommandLine.java:5386)
	at picocli.CommandLine$Help.<init>(CommandLine.java:13145)
	at picocli.CommandLine$DefaultHelpFactory.create(CommandLine.java:4823)
	at picocli.CommandLine$Help.addSubcommand(CommandLine.java:13224)
	at picocli.CommandLine$Help.addAllSubcommands(CommandLine.java:13210)
	at picocli.CommandLine$Help.<init>(CommandLine.java:13151)
	at picocli.CommandLine$DefaultHelpFactory.create(CommandLine.java:4823)
	at picocli.CommandLine.usage(CommandLine.java:2448)
	at picocli.CommandLine.usage(CommandLine.java:2404)
	at picocli.CommandLine.usage(CommandLine.java:2372)
	at picocli.SubcommandTests$Launcher.run(SubcommandTests.java:2156)
	at picocli.CommandLine.executeUserObject(CommandLine.java:1773)
	at picocli.CommandLine.access$900(CommandLine.java:145)
	at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2154)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2148)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2112)
	at picocli.CommandLine$AbstractParseResultHandler.execute(CommandLine.java:1979)
	at picocli.CommandLine.execute(CommandLine.java:1908)
	at picocli.SubcommandTests$Launcher.main(SubcommandTests.java:2152)
Caused by: java.lang.reflect.InvocationTargetException
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at picocli.CommandLine$DefaultFactory.create(CommandLine.java:4923)
	at picocli.CommandLine$DefaultFactory.create(CommandLine.java:4944)
	... 20 more
Caused by: java.lang.IllegalStateException: Don't instantiate me!
	at picocli.SubcommandTests$Foo.<init>(SubcommandTests.java:2137)
	... 26 more

When the usage help message is created, subcommands are instantiated in the Help constructor. I don't remember why I did this. There is a comment referring to this issue so this was done intentionally. Interestingly, if I remove that line so that subcommands are not instantiated in the Help constructor, all picocli tests still pass, so perhaps this is not necessary after all.

I need to look at this further...

@remkop
Copy link
Owner

remkop commented Mar 1, 2020

@Immueggpain Thanks for raising this!

I now believe this is a leftover from early work on this (#690) ticket that was not cleaned up.
The tests all passing even when the user object is not instantiated in the Help constructor supports this.

I will close this ticket; I created a new ticket (#968) for fixing the problem you identified; the fix will be in the next picocli release.

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

3 participants