8296776: Stop using mtNone as marker for CHeap allocations in GrowableArray #11086

stefank · 2022-11-10T09:40:44Z

Today we use mtNone to denote that a GrowableArray should NOT be backed by a CHeap allocated array.

I've gotten feedback that it would probably be good to limit the usage of mtNone, and at some point maybe completely remove it.

This patch takes a small step to remove mtNone from the GrowableArray. What's left is only asserts to forbid that value. Those asserts will be trivial to remove when/if mtNone is removed.

Just like in the proposed patch to make MEMFLAGS non-optional in CHeapBitMap (see JDK-JDK-8296774), I have thrown around the parameter order for GrowableArray. When looking at the changes to the usages of CHeap-backed GrowableArrays it becomes apparent that all of these usages where forced to provide a value for the initial capacity. When MEMFLAGS move to the front, we can now skip having to figure an initial capacity.

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-8296776: Stop using mtNone as marker for CHeap allocations in GrowableArray

Reviewers

Serguei Spitsyn (@sspitsyn - Reviewer) ⚠️ Review applies to 9e398353
Xin Liu (@navyxliu - Committer) ⚠️ Review applies to 7b43d04a
Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to b2cfd572

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11086

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

Using diff file

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

bridgekeeper · 2022-11-10T09:42:03Z

👋 Welcome back stefank! 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 · 2022-11-10T09:43:59Z

@stefank this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8296776_growablearray_mtnone_cleanout
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2022-11-10T09:44:51Z

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

hotspot
serviceability

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 · 2022-11-10T09:47:49Z

Webrevs

…eArray

stefank · 2022-11-10T10:10:07Z

/label remove serviceability

openjdk · 2022-11-10T10:11:08Z

@stefank
The serviceability label was successfully removed.

sspitsyn · 2022-11-10T11:23:26Z

All the serviceability files look okay to me.
Thanks,
Serguei

openjdk · 2022-11-10T11:25:44Z

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

8296776: Stop using mtNone as marker for CHeap allocations in GrowableArray

Reviewed-by: sspitsyn, xliu, stuefe

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

d0fae43: 8294947: Use 64bit atomics in patch_verified_entry on x86_64
6f467cd: 8295934: IGV: keep node selection when changing view or graph
9adb728: 8295070: Introduce more target combinations for compiler flags
8ab70d3: 8294775: Shenandoah: reduce contention on _threads_in_evac

Please see this link for an up-to-date comparison between the source branch of this pull request and the master 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.

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

navyxliu

It looks like no one is using mtNone for GrowableArray, but memReporter.cpp and virtualMemoryTracker still accept it. Shall we eliminate mtNone completely?

navyxliu · 2022-11-11T07:33:46Z

src/hotspot/share/utilities/growableArray.hpp

+    init_checks();
+  }
+
+  GrowableArray(MEMFLAGS memflags, int initial_capacity = 2) :


I feel it's too dangerous to have two constructors with convertible parameters.
eg. GrowableArray(int initial_capacity) and GrowableArray(MEMFLAGS memflags, int initial_capacity = 2) are very close. what if someone has code like this:

short t = 2; GrowableArray(2); // it's very vague to me which ctor will be selected.

I think it's a good idea to use 'explicit constructor' for one of them. in case implicit conversation takes place unconsciously.

MEMFLAGS is defined as a class enum, so integers will not be automatically converted to MEMFLAGS. I'll add explicit to the constructors.

openjdk-notifier · 2022-11-11T08:48:26Z

@stefank Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

stefank · 2022-11-11T11:44:13Z

It looks like no one is using mtNone for GrowableArray, but memReporter.cpp and virtualMemoryTracker still accept it. Shall we eliminate mtNone completely?

Eliminating mtNone completely is out of scope for this PR.

navyxliu

LGTM. I am not a reviewer.

stefank · 2022-11-11T20:57:01Z

Thanks for reviewing!

dholmes-ora · 2022-11-14T00:12:45Z

When looking at the changes to the usages of CHeap-backed GrowableArrays it becomes apparent that all of these usages where forced to provide a value for the initial capacity. When MEMFLAGS move to the front, we can now skip having to figure an initial capacity.

The default initial capacity is 2, the explicit initial capacities are not 2 and in many case >>>2. So without an explicit capacity passed in these GrowableArrays would likely waste a lot of time unnecessarily growing. I don't think either of these parameters should really have a default value - in which case the order could have remained as it was.

stefank · 2022-11-14T10:06:57Z

When looking at the changes to the usages of CHeap-backed GrowableArrays it becomes apparent that all of these usages where forced to provide a value for the initial capacity. When MEMFLAGS move to the front, we can now skip having to figure an initial capacity.

The default initial capacity is 2, the explicit initial capacities are not 2 and in many case >>>2. So without an explicit capacity passed in these GrowableArrays would likely waste a lot of time unnecessarily growing. I don't think either of these parameters should really have a default value - in which case the order could have remained as it was.

Sure, that's an alternative. Before committing to doing that, could you take a look at the similar change to CHeap allocated BitMaps #11084? I want the two classes to be similar in where we put the "allocation strategy". With both these changes I've pushed them to the beginning of the parameter list. With your suggestion I either have to make GrowableArray and CHeapBitMap inconsistent, or I need to change CHeapBitMap to also have the MEMFLAGS at the end, which would force all users to set the clear parameter. I'm fine with either approach, but I'd like to get feedback on that before moving ahead with this PR.

stefank · 2022-11-15T08:35:16Z

I'm going to go with @dholmes-ora suggestion here, and do a similar change to the CHeapBitMap PR.

…rray_mtnone_cleanout_review

tstuefe

Looks good to me. I like this version better. Thanks for doing this.

Small nits remain, nothing big.

src/hotspot/share/utilities/growableArray.hpp

stefank

Thanks for looking at this, Thomas.

I'm fixing most of your suggestions but left bits() alone, since I like it.

src/hotspot/share/utilities/growableArray.hpp

tstuefe · 2022-11-15T11:08:39Z

All good Stefan! I already approved.

Cheers, Thomas

stefank · 2022-11-16T09:15:06Z

Thanks for the reviews!

stefank · 2022-11-16T10:51:15Z

/integrate

openjdk · 2022-11-16T10:53:15Z

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

499406c: 8285488: Improve DocFinder
97ab2c3: 8296953: Fix a typo in exception documentation
5e08b3f: 8296931: NMT tests slowed down considerably by JDK-8242181
65f7de2: 8022403: sun/java2d/DirectX/OnScreenRenderingResizeTest/OnScreenRenderingResizeTest.java fails
3f2f128: 8295952: Problemlist existing compiler/rtm tests also on x86
6ead2b0: 8296548: Improve MD5 intrinsic for x86_64
bd3acbe: 8297089: [BACKOUT] JDK-8297088 Update LCMS to 2.14
6a60d31: 8295369: Update LCMS to 2.14
0cbf084: 8296969: C1: PrintC1Statistics is broken after JDK-8292878
f662a06: 8296970: Remove sysThreadAvailableStackWithSlack from hotspot-symbols
... and 14 more: https://git.openjdk.org/jdk/compare/5551cb66ba58cadce7291a95c5af662ade2cb7b8...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-16T10:53:30Z

@stefank Pushed as commit 5f51dff.

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

dholmes-ora · 2022-11-17T02:06:45Z

Sorry @stefank this one slipped through the cracks. This simplified version looks better to me. Sorry for not responding to your query sooner.

stefank marked this pull request as draft November 10, 2022 09:42

openjdk bot added merge-conflict Pull request has merge conflict with target branch rfr Pull request is ready for review labels Nov 10, 2022

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org and removed rfr Pull request is ready for review labels Nov 10, 2022

stefank force-pushed the 8296776_growablearray_mtnone_cleanout branch from ba3508f to 9e39835 Compare November 10, 2022 10:09

8296776: Stop using mtNone as marker for CHeap allocations in Growabl…

9e39835

…eArray

stefank marked this pull request as ready for review November 10, 2022 10:09

openjdk bot removed merge-conflict Pull request has merge conflict with target branch serviceability serviceability-dev@openjdk.org labels Nov 10, 2022

openjdk bot added the rfr Pull request is ready for review label Nov 10, 2022

sspitsyn approved these changes Nov 10, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 10, 2022

navyxliu reviewed Nov 11, 2022

View reviewed changes

Mark constructors explicit

7b43d04

navyxliu approved these changes Nov 11, 2022

View reviewed changes

Review dholmes

a242ec6

stefank mentioned this pull request Nov 15, 2022

8296774: Removed default MEMFLAGS value from CHeapBitMap #11084

Closed

3 tasks

stefank added 3 commits November 15, 2022 09:39

Merge remote-tracking branch 'upstream/master' into 8296776_growablea…

dfa1171

…rray_mtnone_cleanout_review

Try to fix 32-bit builds

72d580e

Move now unnecessary 'explicit' specifier

b2cfd57

tstuefe approved these changes Nov 15, 2022

View reviewed changes

src/hotspot/share/utilities/growableArray.hpp Outdated Show resolved Hide resolved

src/hotspot/share/utilities/growableArray.hpp Show resolved Hide resolved

src/hotspot/share/utilities/growableArray.hpp Show resolved Hide resolved

Review tstuefe

7a0ab73

stefank commented Nov 15, 2022

View reviewed changes

src/hotspot/share/utilities/growableArray.hpp Outdated Show resolved Hide resolved

src/hotspot/share/utilities/growableArray.hpp Show resolved Hide resolved

Spelling

f5d2440

openjdk bot mentioned this pull request Nov 15, 2022

8297020: Rename GrowableArray::on_stack #11161

Closed

3 tasks

openjdk bot added the integrated Pull request has been integrated label Nov 16, 2022

openjdk bot closed this Nov 16, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 16, 2022

8296776: Stop using mtNone as marker for CHeap allocations in GrowableArray #11086

8296776: Stop using mtNone as marker for CHeap allocations in GrowableArray #11086

stefank commented Nov 10, 2022 •

edited by openjdk bot

bridgekeeper bot commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

mlbridge bot commented Nov 10, 2022 •

edited

stefank commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

sspitsyn commented Nov 10, 2022

openjdk bot commented Nov 10, 2022 •

edited

navyxliu left a comment

navyxliu Nov 11, 2022

stefank Nov 11, 2022 •

edited

openjdk-notifier bot commented Nov 11, 2022

stefank commented Nov 11, 2022

navyxliu left a comment

stefank commented Nov 11, 2022

dholmes-ora commented Nov 14, 2022

stefank commented Nov 14, 2022

stefank commented Nov 15, 2022

tstuefe left a comment

stefank left a comment

tstuefe commented Nov 15, 2022

stefank commented Nov 16, 2022

stefank commented Nov 16, 2022

openjdk bot commented Nov 16, 2022

openjdk bot commented Nov 16, 2022

dholmes-ora commented Nov 17, 2022

8296776: Stop using mtNone as marker for CHeap allocations in GrowableArray #11086

8296776: Stop using mtNone as marker for CHeap allocations in GrowableArray #11086

Conversation

stefank commented Nov 10, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

mlbridge bot commented Nov 10, 2022 • edited

Webrevs

stefank commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

sspitsyn commented Nov 10, 2022

openjdk bot commented Nov 10, 2022 • edited

navyxliu left a comment

Choose a reason for hiding this comment

navyxliu Nov 11, 2022

Choose a reason for hiding this comment

stefank Nov 11, 2022 • edited

Choose a reason for hiding this comment

openjdk-notifier bot commented Nov 11, 2022

stefank commented Nov 11, 2022

navyxliu left a comment

Choose a reason for hiding this comment

stefank commented Nov 11, 2022

dholmes-ora commented Nov 14, 2022

stefank commented Nov 14, 2022

stefank commented Nov 15, 2022

tstuefe left a comment

Choose a reason for hiding this comment

stefank left a comment

Choose a reason for hiding this comment

tstuefe commented Nov 15, 2022

stefank commented Nov 16, 2022

stefank commented Nov 16, 2022

openjdk bot commented Nov 16, 2022

openjdk bot commented Nov 16, 2022

dholmes-ora commented Nov 17, 2022

stefank commented Nov 10, 2022 •

edited by openjdk bot

mlbridge bot commented Nov 10, 2022 •

edited

openjdk bot commented Nov 10, 2022 •

edited

stefank Nov 11, 2022 •

edited