RFR: 8303078: Reduce allocations when pretty printing JCTree during compilation

[Jonathan Gibbons]

On Mon, 20 Feb 2023 14:54:18 GMT, Christoph Dreis <duke at openjdk.org> wrote:

> Hi,
> 
> I've been recently optimizing a few project pipelines and always noticed that their compilation step is somewhat "expensive".  Zooming into the issue always revealed Lombok to be a larger contributor. Unfortunately, I can't get rid of Lombok in this customer project (before you ask).
> 
> Apparently, Lombok is printing the respective `JCTree` here and there to check for type matches. I can't imagine this to be super efficient, but that's how it is at the moment.
> <img width="948" alt="image" src="https://user-images.githubusercontent.com/6304496/220134737-bd9b508c-a908-448b-b0d1-e960db28b24a.png">
> 
> Anyhow, regardless of the Lombok inefficiencies I think there are some optimization opportunities in the JDK itself. 
> 
> 1. Overall, `Pretty.visitSelect` accounts for 8-10% of the total allocations in this project. And among those there are StringBuilder allocations coming from the following:
> 
> 
>     public void visitSelect(JCFieldAccess tree) {
>         try {
>             printExpr(tree.selected, TreeInfo.postfixPrec);
>             // StringBuilder allocations hiding in here.
>             print("." + tree.name);
>         } catch (IOException e) {
>             throw new UncheckedIOException(e);
>         }
>     }
> 
> 
> This PR splits the `print` calls into two separate ones to avoid this String concatenation.
> 
> ...
>             printExpr(tree.selected, TreeInfo.postfixPrec);
>             print('.');
>             print(tree.name);
> ...
> 
> 
> Secondly, the `print` method takes an `Object` which seems like a good fit for another (private?) variant of it that only takes a `char`. By this we would probably avoid any eventual boxing and avoid any conversion with `Convert.escapeUnicode(s.toString())` that seems superfluous for chars like `.`, ` `, or any braces like `(`, `{` etc. 
> 
> This is currently a draft PR as long as the scope is not clarified. It currently only includes the necessary changes that would optimize the particular use-case. But there are more cases where e.g. the new `char` variant could be used and/or any String concatenation could be split into separate `print` calls.
> 
> Let me know what you think and if I should include the other cases as well. If you think this is worthwhile, I'd appreciate if this is is sponsored. (Including creating an issue as I can't do this myself apparently. I will of course squash everything together with the proper issue ID once available.)
> 
> I've contributed before, so the CLA should be signed.
> 
> Cheers,
> Christoph

If we proceed with this, I would recommend looking for other possible uses of the new method.

A simple `grep` shows 129 single character strings, of which 97 are arguments to `print`.

Separate but related, I note that a similar class, `DocPretty.java`, has 75 single character strings, of which  68 are arguments to print.

Overall changing the coding style to use `out.print(char)` seems worthwhile.

Filed [JDK-8303078](https://bugs.openjdk.org/browse/JDK-8303078)

You might want to un-Draft the PR, and prefix the title with `JDK-8303078: `

There are 19 instances of string concatenation in `print` arguments, for your consideration as well.

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/Pretty.java line 139:

> 137:     private void print(char c) throws IOException {
> 138:         out.write(c);
> 139:     }

This bypasses the `Convert.escapeUnicode` code on line 132.   That's not bad, but should be clearly documented ... in that this is a special method for known ASCII characters, and not a general purpose method that could be used on the characters of a string

-------------

PR: https://git.openjdk.org/jdk/pull/12667