8276904: Optional.toString() is unnecessarily expensive

[eamonnmcmanus]

Use string concatenation instead of String.format.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8276904: Optional.toString() is unnecessarily expensive

Reviewers

• Roger Riggs (@RogerRiggs - Reviewer)
• Stuart Marks (@stuart-marks - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6337/head:pull/6337
$ git checkout pull/6337

Update a local copy of the PR:
$ git checkout pull/6337
$ git pull https://git.openjdk.java.net/jdk pull/6337/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6337

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6337.diff

[bridgekeeper]

👋 Welcome back emcmanus! 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]

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

• core-libs

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.

[mlbridge]

Webrevs

• 00: Full (b44052b9)

[mlbridge]

Mailing list message from Brian Goetz on core-libs-dev:

I would suggest that we hold this patch until the string interpolation
JEP lands.? It will offer both better readability *and* better
performance, and probably better to have one round of "replace all the
toString implementations" than two?

On 11/10/2021 1:04 PM, Eamonn McManus wrote:

[stuart-marks]

I talked to Brian about this a bit. Changing format to concatenation is probably ok this case for two reasons: 1) it's the simplest possible such transformation, and as such it's not a very good example for the templating stuff; and 2) presumably you bothered filing the bug and the PR because you believe the performance improvement would be noticeable in actual systems.

I note that some Amazon folks had filed JDK-8275190 for what is essentially the same issue. (I closed it as a dupe.) They did include some performance numbers, but not the actual code they measured, so without guessing it's hard to say what they actually measured. Could you run some numbers and post them, and can you share some estimates of the impact it might have on your systems?

The reason I'm interested in some data here is that this sort of change is worth doing only if it has a real, measurable impact. In turn that implies that you're probably using Optional::toString a lot. That's an important criterion; we don't want to do a wholesale replacement of format with concatenation in the JDK, based on some micro benchmarks, that end up garbaging up the JDK code base while not actually improving any real systems.

[RogerRiggs]

Looks ok on the face of it. I would be useful to see the performance of the improvement.

[eamonnmcmanus]

Google has profiling in place on its major Java server processes. We observe that, across all profiled Optional.toString() calls, about 25% of the total CPU time from those calls is spent in Formatter.parse and the methods it calls. This includes cases where the toString() of the underlying object is itself quite expensive, so the 25% should be seen as a lower bound. It also doesn't include the other methods that String.format ends up calling.

If the embedded object has a cheap toString(), Formatter.parse can end up being 90% of the CPU time.

As a separate but larger project, I think it would be worth investigating whether Formatter can be rewritten so that it doesn't use regular expressions to parse the template strings. Users are generally surprised at how expensive Formatter (and therefore String.format) can be. But meanwhile I think the much simpler change here is already beneficial.

[openjdk]

@eamonnmcmanus 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:

8276904: Optional.toString() is unnecessarily expensive

Reviewed-by: rriggs, smarks

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 401 new commits pushed to the master branch:

• 92aa75b: 8274903: Zero: Support AsyncGetCallTrace
• 8345453: 8272392: Lanai: SwingSet2. Black background on expanding tree node
• 9b74749: 8276660: Scalability bottleneck in java.security.Provider.getService()
• 2478158: 8277361: java/nio/channels/Channels/ReadXBytes.java fails with OOM error
• 8af3b27: 8277850: C2: optimize mask checks in counted loops
• 3e93e0b: 8276769: -Xshare:auto should tolerate problems in the CDS archive
• 79165b7: 8278324: Update the --generate-cds-archive jlink plugin usage message
• 40d726b: 8278310: Improve logging in CDS DynamicLoaderConstraintsTest.java
• e4852c6: 8277998: runtime/cds/appcds/loaderConstraints/DynamicLoaderConstraintsTest.java#custom-cl-zgc failed "assert(ZAddress::is_marked(addr)) failed: Should be marked"
• 37921e3: 8269258: java/net/httpclient/ManyRequestsLegacy.java failed with connection timeout
• ... and 391 more: https://git.openjdk.java.net/jdk/compare/ce3ed65ac3411a533052a8c01231f7e540803afb...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[stuart-marks]

To reiterate what I said previously, I think these changes are OK because they're the simplest possible case for string concatenation vs. formatting, and therefore the future templating stuff is unlikely to help much; and this has been observed at least twice as the cause of noticeable performance issues (by Google and Amazon).

There are a variety of things that could be done in general to speed up formatting. One is straight-up optimization of Formatter.parse, for example, to avoid regexes. Another is to speed up regexes! A third is to pre-compile format strings; it might be reasonable to dust off work that's been done previously in this area. A fourth is the string templating work (which might subsume the previous item).

[eamonnmcmanus]

/integrate

[openjdk]

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

• 6e7b7f3: 8278251: Enable "missing-explicit-ctor" check in the jdk.unsupported.desktop module
• c83b781: 8278459: ProblemList javax/swing/JTree/4908142/bug4908142.java on macosx-aarch64
• 8e8fadf: 8278428: ObjectInputStream.readFully range check incorrect
• 5a80abf: 8272944: Use snippets in jdk.javadoc documentation
• fb11d8f: 8272945: Use snippets in java.compiler documentation
• 42d9b1b: 8277106: Cannot compile certain sources with --release
• ba86dd4: 8278445: ProblemList tools/jpackage/share/IconTest.java on macosx-x64
• 92aa75b: 8274903: Zero: Support AsyncGetCallTrace
• 8345453: 8272392: Lanai: SwingSet2. Black background on expanding tree node
• 9b74749: 8276660: Scalability bottleneck in java.security.Provider.getService()
• ... and 398 more: https://git.openjdk.java.net/jdk/compare/ce3ed65ac3411a533052a8c01231f7e540803afb...master

Your commit was automatically rebased without conflicts.

[openjdk]

@eamonnmcmanus Pushed as commit fe2ae8e.

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