8303078: Reduce allocations when pretty printing JCTree during compilation #12667

dreis2211 · 2023-02-20T14:54:18Z

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.

Anyhow, regardless of the Lombok inefficiencies I think there are some optimization opportunities in the JDK itself.

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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8303078: Reduce allocations when pretty printing JCTree during compilation

Reviewers

Jonathan Gibbons (@jonathan-gibbons - Reviewer)
Vicente Romero (@vicente-romero-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12667/head:pull/12667
$ git checkout pull/12667

Update a local copy of the PR:
$ git checkout pull/12667
$ git pull https://git.openjdk.org/jdk pull/12667/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12667

View PR using the GUI difftool:
$ git pr show -t 12667

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12667.diff

bridgekeeper · 2023-02-20T14:55:37Z

👋 Welcome back dreis2211! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-02-20T14:57:10Z

@dreis2211 The following label will be automatically applied to this pull request:

compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

dreis2211 · 2023-02-22T12:10:32Z

With the patch applied I can reduce things for an internal product.

Some specs about the project:

~2500 classes
Average size: ~76 lines
Largest class: ~2500 lines
Mostly hand-written, but some auto-generated protobuf files (I'd say 90/10 split)

CPU profile before

CPU profile after

Allocation profile before

Allocation profile after

Mode	Frames matched before	Frames matched after
`cpu`	2.27%	1.42%
`alloc`	7.09%	4.13%

In terms of timings I couldn't measure anything substantially on the actual project. Neither a regression, nor a substantial improvement. But it's not so much about time anyhow but about the allocations here. The overall goal is to reduce impact on GC that is using 27% of the CPU frames (to give you some more background). Obviously, this is not coming from the JDK alone and Lombok is a major contributor here, but the JDK can ease the impact of Lombok here I'd say.

Admittedly, it's more on the smaller end of things and a smaller knob in terms of GC improvements.

Let me know if you need more information.

dreis2211 · 2023-02-22T12:36:01Z

In terms of some more absolute allocation numbers:

Before (TOP 3 => ~5,6GB)

       bytes  percent  samples  top
  ----------  -------  -------  ---
  3581028736   26.79%    10682  byte[]
  1291159216    9.66%     3930  java.lang.Object[]
   740065552    5.54%     2161  java.lang.String

After (TOP 3 => ~5,4GB)

      bytes  percent  samples  top
  ----------  -------  -------  ---
  3376915824   26.33%    11615  byte[]
  1284160152   10.01%     3847  java.lang.Object[]
   730100712    5.69%     2023  java.lang.String

StringBuilder savings account for an additional 62MB

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

jonathan-gibbons

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.

dreis2211 · 2023-02-22T14:55:16Z

@jonathan-gibbons Thanks for your review.

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

That's the idea - there are multiple cases where this could be used. I'll push an updated version that already changes things to make reviewing a bit easier.

EDIT: Done.

jonathan-gibbons · 2023-02-22T15:06:55Z

Filed JDK-8303078

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

jonathan-gibbons · 2023-02-22T15:13:26Z

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

dreis2211 · 2023-02-22T15:28:44Z

@jonathan-gibbons Made the adjustments to Pretty.

openjdk · 2023-02-22T15:32:09Z

@dreis2211 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

mlbridge · 2023-02-22T15:35:06Z

Webrevs

vicente-romero-oracle

this is looking good, I think that once the changes to DocPretty.java, as Jon suggested, are included in this patch it should be good to go

dreis2211 · 2023-02-22T16:32:44Z

@vicente-romero-oracle & @jonathan-gibbons made the adjustments to DocPretty now as well.

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

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

dreis2211 · 2023-02-22T22:07:09Z

There are some test failures in langtools/tier1 that seem related. Will check

jonathan-gibbons · 2023-02-23T01:21:42Z

I confirm that all the langtools tests now pass, at least on my local laptop.

jonathan-gibbons

Thanks for updating DocPretty.java to follow the same conventions as Pretty.java, even though it was probably not required for your Lombok-based use-case.

I've gone through all the latest patch, and run all the langtools tests as well. All pass on my laptop, and there is no reason to expect any cross-platform issues.

I'll mark it Approved, but while not strictly necessary, I recommend that you wait for Vicente's approval as well, in case he has any final comments.

openjdk · 2023-02-23T01:35:21Z

@dreis2211 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8303078: Reduce allocations when pretty printing JCTree during compilation

Reviewed-by: jjg, vromero

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 66 new commits pushed to the master branch:

23e9d9d: 8302811: NMT.random_reallocs_vm fails if NMT is off
fcaf871: 8302028: Port fdlibm atan2 to Java
07e976a: 8303081: Serial: Remove unused VM_MarkSweep
8de841d: 8302667: Improve message format when failing to load symbols or libraries
f893d23: 8303024: (fs) WindowsFileSystem.supportedFileAttributeViews can use Set.of
d7ada66: 8303084: G1 Heap region liveness verification has inverted return value
5d7e7e2: 8302760: Improve liveness/remembered set verification for G1
b0e0f37: 8303067: G1: Remove unimplemented G1FullGCScope::heap_transition
1a62a12: 8302880: Fix includes in g1ConcurrentMarkObjArrayProcessor files
ee37af4: 8302975: Remove redundant mark verification during G1 Full GC
... and 56 more: https://git.openjdk.org/jdk/compare/71cf7c4409025c87ac786a54171f00de69fe5317...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jonathan-gibbons, @vicente-romero-oracle) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

vicente-romero-oracle

looks good. I will run an internal test that executes all regression tests in all supported platforms just to be xtra sure

vicente-romero-oracle · 2023-02-23T04:54:00Z

looks good. I will run an internal test that executes all regression tests in all supported platforms just to be xtra sure

all green :)

dreis2211 · 2023-02-23T07:03:52Z

/integrate

openjdk · 2023-02-23T07:05:09Z

@dreis2211
Your change (at version 03e9893) is now ready to be sponsored by a Committer.

mlbridge · 2023-02-23T07:16:48Z

Mailing list message from Jonathan Gibbons on compiler-dev:

Regarding paranoia: I'll address that in the PR, I'd like to record the
suggestion there, but probably agree with your suggestion to leave it as is.

Regarding the scary comment: the comment goes back to pre-module days,
when more people were helping themselves to using notionally internal
JDK API.? Now that the internals are encapsulated, the scary comment
seems less important, and might even go away, or be replaced by a single
notice higher up. (It's helpful to have a place we can point to that
says, "you were warned...").? We have already removed the scary warning
from the `jdk.javadoc` module, but that code has the added advantage of
having `.internal.` in the name of the relevant packages.

-- Jon

On 2/22/23 11:02 AM, Christoph Dreis wrote:

jonathan-gibbons · 2023-02-23T15:23:52Z

/sponsor

openjdk · 2023-02-23T15:25:19Z

Going to push as commit 58ca711.
Since your change was applied there have been 70 commits pushed to the master branch:

2cf8b86: 8303071: Memory leaks in libjdwp
f113b04: 8302927: Unproblemlist Fuzz.java from ProblemList-zgc.txt again
1bab93b: 8303073: (bf) Temporarily reinstate private DirectByteBuffer(long, int) constructor
221f346: 8303047: avoid NULL after 8301661
23e9d9d: 8302811: NMT.random_reallocs_vm fails if NMT is off
fcaf871: 8302028: Port fdlibm atan2 to Java
07e976a: 8303081: Serial: Remove unused VM_MarkSweep
8de841d: 8302667: Improve message format when failing to load symbols or libraries
f893d23: 8303024: (fs) WindowsFileSystem.supportedFileAttributeViews can use Set.of
d7ada66: 8303084: G1 Heap region liveness verification has inverted return value
... and 60 more: https://git.openjdk.org/jdk/compare/71cf7c4409025c87ac786a54171f00de69fe5317...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-23T15:25:43Z

@jonathan-gibbons @dreis2211 Pushed as commit 58ca711.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

dreis2211 · 2023-02-23T15:26:44Z

Thanks @jonathan-gibbons & @vicente-romero-oracle for your active advice, reviewing and sponsoring :)

jonathan-gibbons · 2023-02-23T15:35:27Z

Thanks @jonathan-gibbons & @vicente-romero-oracle for your active advice, reviewing and sponsoring :)

You're welcome; thanks for the contribution.

vicente-romero-oracle · 2023-02-23T16:33:02Z

Thanks @jonathan-gibbons & @vicente-romero-oracle for your active advice, reviewing and sponsoring :)

You're welcome; thanks for the contribution.

+1

openjdk bot added the compiler compiler-dev@openjdk.org label Feb 20, 2023

jonathan-gibbons reviewed Feb 22, 2023

View reviewed changes

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/Pretty.java Show resolved Hide resolved

jonathan-gibbons reviewed Feb 22, 2023

View reviewed changes

dreis2211 force-pushed the jctree-pretty-print-allocations branch from c4a003c to 846c8eb Compare February 22, 2023 15:19

dreis2211 changed the title ~~Reduce allocations when pretty printing JCTree during compilation~~ 8303078: Reduce allocations when pretty printing JCTree during compilation Feb 22, 2023

dreis2211 force-pushed the jctree-pretty-print-allocations branch from 846c8eb to 3efe9d5 Compare February 22, 2023 15:23

dreis2211 marked this pull request as ready for review February 22, 2023 15:28

8303078: Reduce allocations when pretty printing JCTree

45d1219

dreis2211 force-pushed the jctree-pretty-print-allocations branch from 3efe9d5 to 45d1219 Compare February 22, 2023 15:31

openjdk bot added the rfr Pull request is ready for review label Feb 22, 2023

vicente-romero-oracle reviewed Feb 22, 2023

View reviewed changes

8303078: Reduce allocations for single characters in DocPretty

b1b8d0c

vicente-romero-oracle reviewed Feb 22, 2023

View reviewed changes

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/Pretty.java Show resolved Hide resolved

vicente-romero-oracle reviewed Feb 22, 2023

View reviewed changes

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/Pretty.java Outdated Show resolved Hide resolved

8303078: Replace some more character print calls

f5c5db7

vicente-romero-oracle reviewed Feb 22, 2023

View reviewed changes

8303078: Replace some more character print calls

34f9c33

jonathan-gibbons reviewed Feb 22, 2023

View reviewed changes

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocPretty.java Show resolved Hide resolved

jonathan-gibbons reviewed Feb 22, 2023

View reviewed changes

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocPretty.java Outdated Show resolved Hide resolved

8303078: Make method private

489e3fe

8303078: Fix Pretty.visitLambda

03e9893

jonathan-gibbons approved these changes Feb 23, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 23, 2023

vicente-romero-oracle approved these changes Feb 23, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 23, 2023

openjdk bot added the integrated Pull request has been integrated label Feb 23, 2023

openjdk bot closed this Feb 23, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Feb 23, 2023

8303078: Reduce allocations when pretty printing JCTree during compilation #12667

8303078: Reduce allocations when pretty printing JCTree during compilation #12667

dreis2211 commented Feb 20, 2023 •

edited by openjdk bot

bridgekeeper bot commented Feb 20, 2023

openjdk bot commented Feb 20, 2023

dreis2211 commented Feb 22, 2023 •

edited

dreis2211 commented Feb 22, 2023

jonathan-gibbons left a comment •

edited

dreis2211 commented Feb 22, 2023 •

edited

jonathan-gibbons commented Feb 22, 2023 •

edited

jonathan-gibbons commented Feb 22, 2023

dreis2211 commented Feb 22, 2023

openjdk bot commented Feb 22, 2023

mlbridge bot commented Feb 22, 2023 •

edited

vicente-romero-oracle left a comment

dreis2211 commented Feb 22, 2023

dreis2211 commented Feb 22, 2023

jonathan-gibbons commented Feb 23, 2023

jonathan-gibbons left a comment

openjdk bot commented Feb 23, 2023 •

edited

vicente-romero-oracle left a comment

vicente-romero-oracle commented Feb 23, 2023

dreis2211 commented Feb 23, 2023

openjdk bot commented Feb 23, 2023

mlbridge bot commented Feb 23, 2023

jonathan-gibbons commented Feb 23, 2023

openjdk bot commented Feb 23, 2023

openjdk bot commented Feb 23, 2023

dreis2211 commented Feb 23, 2023

jonathan-gibbons commented Feb 23, 2023

vicente-romero-oracle commented Feb 23, 2023

8303078: Reduce allocations when pretty printing JCTree during compilation #12667

8303078: Reduce allocations when pretty printing JCTree during compilation #12667

Conversation

dreis2211 commented Feb 20, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 20, 2023

openjdk bot commented Feb 20, 2023

dreis2211 commented Feb 22, 2023 • edited

dreis2211 commented Feb 22, 2023

jonathan-gibbons left a comment • edited

Choose a reason for hiding this comment

dreis2211 commented Feb 22, 2023 • edited

jonathan-gibbons commented Feb 22, 2023 • edited

jonathan-gibbons commented Feb 22, 2023

dreis2211 commented Feb 22, 2023

openjdk bot commented Feb 22, 2023

mlbridge bot commented Feb 22, 2023 • edited

Webrevs

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

dreis2211 commented Feb 22, 2023

dreis2211 commented Feb 22, 2023

jonathan-gibbons commented Feb 23, 2023

jonathan-gibbons left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 23, 2023 • edited

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

vicente-romero-oracle commented Feb 23, 2023

dreis2211 commented Feb 23, 2023

openjdk bot commented Feb 23, 2023

mlbridge bot commented Feb 23, 2023

jonathan-gibbons commented Feb 23, 2023

openjdk bot commented Feb 23, 2023

openjdk bot commented Feb 23, 2023

dreis2211 commented Feb 23, 2023

jonathan-gibbons commented Feb 23, 2023

vicente-romero-oracle commented Feb 23, 2023

dreis2211 commented Feb 20, 2023 •

edited by openjdk bot

dreis2211 commented Feb 22, 2023 •

edited

jonathan-gibbons left a comment •

edited

dreis2211 commented Feb 22, 2023 •

edited

jonathan-gibbons commented Feb 22, 2023 •

edited

mlbridge bot commented Feb 22, 2023 •

edited

openjdk bot commented Feb 23, 2023 •

edited