fusesource / jansi

Jansi is a small java library that allows you to use ANSI escape sequences to format your console output which works even on windows.
http://fusesource.github.io/jansi/
Apache License 2.0
1.11k stars 140 forks source link

big performance improvment + refactor #141

Closed Arnaud-Nauwynck closed 5 years ago

Arnaud-Nauwynck commented 5 years ago

initially, this pull request was typo "!" IS_CYGWIN = ... !"cygwin".equals(System.getenv("TERM") instead of IS_CYGWIN = ... "cygwin".equals(System.getenv("TERM")

notice that in a windows cmd.exe console, the jansi mode "WINDOWS" works fine ... this is independent of the fact that the process "bash.exe" from cygwin is running

notice also that on windows, a very frequent usage is to use cmder console emulator (https://cmder.net/) + cygwin The cmder console define several environment variables, including ConEmuANSI="ON" which could be tested to use the standard ANSI mode of jansi

.......... After that, I started working on the performance bottleneck of excessive flush() called, and refactoring for duplicate code (unfortunatly, I did it just after, so on the same branch... same PullRequest)

I have opened several tickets explaining the performance problems of ~10 times slower code with JAnsi compared to raw System.out

https://github.com/fusesource/jansi/issues/144 https://github.com/fusesource/jansi/issues/143 https://github.com/fusesource/jansi/issues/142 There is my benchmark code here https://github.com/Arnaud-Nauwynck/test-snippets/blob/master/test-bench-jansi/src/main/java/org/fusesource/jansi/BenchmarkMain.java

What I did to improve performances / code: I found the class PrintStream is NOT necessary to extends as AnsiPrintStream. It is simpler to reuse plain-old java.io.PrintStream which does correct formatting and synchronized stuff... then delegate all filtering job to a FilterOutputStream, so keeping the class AnsiOutputStream. But, following good design SOLID principles, it is better to delegate all "process*()" methods for terminal specific handling into a separate class. Therefore, I introduced the class TerminalCommandProcessor which, as it names stands for only "process commands for the terminal". This clear separation of PrintStream / JAnsiOutputSteam / TerminalCommandProcessor allows much cleaner code (altough it is quite tricky to keep a reference to the stream from the processor... in particular when overriding for html, cf using inner class init from constructor()) So finally, I could remove all the code duplication that was considerably complexyfing the library

For final performance improvement, I have noticed that methods like "write(byte[])" or "write(byte[], offset, len)" are transformed to single byte system calls "write(int)" so it is NECESSARY to rewrap the underlying OutputStream with a BufferedOutputStream, to perform fewer small systems calls

I have tested on my windows PC, under cygwin / cmd.exex / cmder terminal It is now FAAAAAST again !!!!

Unfortunatly, it seems that there is a regression on the format of the Jansi initial logo (in the self-excutable jar).. maybe a problem with a missing flush(), or resetAttributes() or an escape ansi sequence badly interpreted? So there is a problem, but I haven't figured it out yet ... Please help if you have an idea.

hboutemy commented 5 years ago

please split commits into separate PRs that can be reviewed separately and at first look, the second commit could also be split in multiple more focused updates

Arnaud-Nauwynck commented 5 years ago

will rewrite PR in separate PRs

hboutemy commented 5 years ago

thank you