RFR: 8303078: Reduce allocations when pretty printing JCTree during compilation [v3]

[Jonathan Gibbons]

On Wed, 22 Feb 2023 18:22:11 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
>
> Christoph Dreis has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - 8303078: Replace some more character print calls
>  - 8303078: Replace some more character print calls

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocPretty.java line 82:

> 80:      */
> 81:     private void print(char c) throws IOException {
> 82:         out.write(c);

The restriction to skip calling `Convert.escapeUnicode(s.toString())` leaks into `print(List, char sep)`.

If we wanted to be paranoid we could do one or more of the following;

1. Change the body of this method to something like<br> `if (Character.isASCII(c)) out.write(c); else print((Character) c);`
2. include an `assert` or call to a method in `Assert.*` to verify the argument is ASCII.
3. change that `protected void print(List, char)` method to be `private` instead of `protected`

I guess I'd not bother with 1 or 2, but maybe go with 3, since it has no effect on performance, and better restricts the functionality to just this class.   I note there are no discernable external usages of this method.

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

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