8386509: Avoid creating exceptions if they won't be thrown

[raneashay]

Every time an exception object is created, the constructor calls
fillInStackTrace(), which has to walk the call stack to record every
frame. This is expensive, and it's also unnecessary if the exception is
never thrown. There are a few instances in various parts of the JDK
libraries where we create an exception in the dominator block but don't
always throw it. This patch fixes those cases so that if the exception
is not going to be thrown, it is never created in the first place.

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

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-8386509: Avoid creating exceptions if they won't be thrown (Task - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 31487

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@raneashay The following labels will be automatically applied to this pull request:

• compiler
• core-libs
• security

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

[mlbridge]

Webrevs

• 01: Full - Incremental (bfdc49e7)
• 00: Full (1e39b23e)

[AlanBateman]

Ashay - did you use a scanning tool to find these? The motivation seems to be performance but it's not clear that there is anything performance critical here (esp. with the changes to keytool and jshell init).

The exception when using LDAP over TLS fails is probably something for a separate issue/discussion.

[ExE-Boss]

This should probably include the SSLPeerUnverifiedException as a suppressed exception of the CommunicationException:

Suggested change

	} catch (SSLPeerUnverifiedException ex) {
	CommunicationException ce = new CommunicationException();
	ce.setRootCause(closureReason);
	tlsHandshakeCompleted.completeExceptionally(ex);
	tlsHandshakeCompleted.completeExceptionally(ce);
	}
	} catch (SSLPeerUnverifiedException ex) {
	CommunicationException ce = new CommunicationException();
	ce.setRootCause(closureReason);
	ce.addSuppressed(ex);
	tlsHandshakeCompleted.completeExceptionally(ce);
	}

[alexeysemenyukoracle]

FWIW, the intent behind having a single site where an exception is created is to avoid repetitive throw new IOException(rb.getString("Illegal.startdate.value")) calls. The implementation of the intent came with the cost, which this PR mitigates, but defeats the intent :)

I'd create a helper

private static IOException createIllegalStartDateException() {
    return new IOException(rb.getString("Illegal.startdate.value"));
}

And replace every

throw ioe;

with

throw createIllegalStartDateException();

This way, the issue of creating exceptions that won't be thrown is addressed without code duplication.

Alternatively, to keep the fix local, you can use a lambda instead of the helper method:

Supplier<IOException> ioe = () -> {
    return new IOException(rb.getString("Illegal.startdate.value"));
};

Usage:

throw ioe.get();

[raneashay]

Thanks all for taking a look and for suggesting improvements!

Alan, regarding your question, I used a crude Perl script (too big to post here) that essentially looks for the cases where the block that throws the exception is different from the block that creates the exception. I haven't measured the impact of these changes or even whether the fixes are necessary; I saw a similar fix in a downstream fork and decided to stamp it to other portions of the code. I'm seeing this as largely a lint/style issue but if it doesn't seem that important, I am happy to close this PR and/or address the LDAP/TLS problem in a separate issue/PR.