8386963: [lworld] Improve the exception message from Object synchronization methods on value objects

[jaikiran]

Can I please get a review of this change which proposes to improve the exception message in IllegalMonitorStateException when it gets thrown for value objects?

With this change, we now report the following exception message when value objects are used to invoke Object.wait() and related methods:

jshell> o.wait()
|  Exception java.lang.IllegalMonitorStateException: current thread is not owner
|        at Object.wait0 (Native Method)
|        at Object.wait (Object.java:433)
|        at Object.wait (Object.java:387)
|        at (#2:1)

There were some thoughts about using an exception message of the form Exception java.lang.IllegalMonitorStateException: object's type java.lang.Integer is not an identity class. But since value objects cannot be synchronized upon, we decided it would be fine (and accurate) to reuse the same "current thread is not owner" exception message that we use for identity objects.

Given the nature of this change, I haven't added any new jtreg tests for this. Existing tests in tier1, tier2 and tier3 continue to pass after this change.

---

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

---

Progress

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

Issue

• JDK-8386963: [lworld] Improve the exception message from Object synchronization methods on value objects (Enhancement - P4)

Reviewers

• David Holmes (@dholmes-ora - Committer)
• Alan Bateman (@AlanBateman - no project role)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2568

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2568.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

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

8386963: [lworld] Improve the exception message from Object synchronization methods on value objects

Reviewed-by: dholmes, alanb

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 1 new commit pushed to the lworld branch:

• 42fc9c9: 8386999: [lworld] C2: assert(is_dead_loop_safe()) failed: shouldn't be cleared yet

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
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 (@dholmes-ora) 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).

[mlbridge]

Webrevs

• 03: Full - Incremental (38c1cfd1)
• 02: Full - Incremental (d693fcbc)
• 01: Full - Incremental (58246956)
• 00: Full (f6939441)

[AlanBateman]

Is the RM still needed?

[jaikiran]

I wasn't aware what a ResourceMark is. Reading the code and the code comment on ResourceMarkImpl, I believe it was here for tracking the resource allocated in the implementation of obj->klass()->external_name() (please correct me if I'm wrong). With that now gone, I think the ResourceMark is no longer needed. I've updated the PR accordingly.

I have triggered tier testing to be sure that this doesn't cause any issues.

[jaikiran]

tier1, tier2, tier3 testing of the latest changes in this PR completed without any related failures.

[jaikiran]

There's been some discussion on whether to throw an IdentityException instead of IllegalMonitorStateException from these Object.wait()/notify/notifyAll methods when invoked on value objects. There hasn't been any consensus yet and for now it seems that we will continue with the IllegalMonitorStateException being thrown.

I don't have any more changes planned for this PR. The change here will merely improve the exception message contained in the IllegalMonitorStateException. I think that part is trivial and several of us agree that the new message is more appropriate.

The other change in this PR is the removal of ResourceMark from these if blocks. I don't have prior knowledge of that construct, so I would like a review from Alan and/or someone who knows more about this so that we can have this change integrated into lworld in preparation for the JEP 401 integration.

[dholmes-ora]

This seems fine to me. The ResourceMarks are no longer needed.

[jaikiran]

Thank you David and Alan for the reviews.

[jaikiran]

/integrate

[openjdk]

@jaikiran
Your change (at version 38c1cfd) is now ready to be sponsored by a Committer.

[liach]

/sponsor

[openjdk]

Going to push as commit 6f7797c.
Since your change was applied there have been 3 commits pushed to the lworld branch:

• b77db44: 8387300: [lworld] Minor review comments in javac
• 59019cf: 8387192: [lworld] Review comment drop for core libs
• 42fc9c9: 8386999: [lworld] C2: assert(is_dead_loop_safe()) failed: shouldn't be cleared yet

Your commit was automatically rebased without conflicts.

[openjdk]

@liach @jaikiran Pushed as commit 6f7797c.

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