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

Fixed isJansiConsoleInstalled performance issue #1976

Merged
merged 8 commits into from Mar 24, 2023

Conversation

ChrisTrenkamp
Copy link
Contributor

Fixes #1975

isJansiConsoleInstalled() was repeatedly called
in a loop from the TextTable#toString method,
which caused performance problems due to the
constant calls to Class#forName. This change
caches the result from isJansiConsoleInstalled so
it doesn't repeatedly call Class#forName, which
cannot change at runtime.

isJansiConsoleInstalled() was repeatedly called
in a loop from the TextTable#toString method,
which caused performance problems due to the
constant calls to Class#forName.  This change
caches the result from isJansiConsoleInstalled so
it doesn't repeatedly call Class#forName, which
cannot change at runtime.
@remkop remkop added this to the 4.7.2 milestone Mar 20, 2023
@remkop remkop added type: enhancement ✨ theme: usagehelp An issue or change related to the usage help message type: performance ⏱ Performance-related labels Mar 20, 2023
remkop
remkop previously approved these changes Mar 20, 2023
@remkop
Copy link
Owner

remkop commented Mar 20, 2023

@ChrisTrenkamp I had to enable CI/CD for this PR, but now I see some tests are failing:

## On Java 7 Ubuntu
picocli.HelpAnsiHeuristicsTest > testAnsiAutoJansiConsoleInstalledOverridesHintDisabled FAILED
    java.lang.AssertionError at HelpAnsiHeuristicsTest.java:373

and

## On Java 18 Windows
picocli.HelpAnsiTest > testAnsiEnabled FAILED
    java.lang.AssertionError at HelpAnsiTest.java:132

This may be due to the order in which tests are executed.
Perhaps all JansiConsoleInstalled-related tests need to be moved to separate test classes (a separate class per test)?

Fixed unit test by resetting the cached isJansiConsoleInstalled result.
@ChrisTrenkamp
Copy link
Contributor Author

The failing test cases are passing now, but it had to be done by resetting the cached value. Doesn't feel like the most elegant way to handle this.

@remkop
Copy link
Owner

remkop commented Mar 21, 2023

I see what you mean but I cannot think of an alternative either. 😅
I think this is fine.
Thank you fixing the tests!

@remkop
Copy link
Owner

remkop commented Mar 21, 2023

Having some trouble with the CI/CD, but I do see this failing test:

picocli.HelpAnsiHeuristicsTest > testAnsiAutoJansiConsoleInstalledOverridesHintDisabled FAILED
    java.lang.AssertionError at HelpAnsiHeuristicsTest.java:378

@remkop remkop merged commit 01b06bc into remkop:main Mar 24, 2023
24 of 42 checks passed
remkop added a commit that referenced this pull request Mar 24, 2023
@remkop
Copy link
Owner

remkop commented Mar 24, 2023

Merged. Sorry, it took some time to sort out the CI/CD issues.
Many thanks for the contribution!

remkop added a commit that referenced this pull request Mar 24, 2023
remkop added a commit that referenced this pull request Apr 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: usagehelp An issue or change related to the usage help message type: enhancement ✨ type: performance ⏱ Performance-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

~150 millisecond hotspot under the picocli.CommandLine$Help$Ansi$Text.toString method
2 participants