8355013: GrowableArray default constructor should not allocate #24748
Conversation
|
👋 Welcome back qamai! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
merykitty
force-pushed
the
vectordefaultconstructor
branch
from
3c27eec to
af6f640
Compare
|
@merykitty The following label will be automatically applied to this pull request:
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. |
|
@merykitty 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. |
Webrevs
|
| @@ -435,6 +441,10 @@ class GrowableArrayTest : public ::testing::Test { | |||
| } | |||
| }; | |||
|
|
|||
| // static empty vector | |||
| const GrowableArray<int> empty_array(mtTest); | |||
| const GrowableArrayCHeap<int, mtTest> empty_cheap_array; | |||
There was a problem hiding this comment.
Thanks for your reviews, these are additional to ensure that GrowableArray instances can be created in static context. I have added a test to check these are truly empty.
There was a problem hiding this comment.
HotSpot denigrates non-local objects with non-trivial ctors. We shouldn't have such instances.
https://github.com/openjdk/jdk/blame/4dd64b49716144cc697fb461ff88860e2cbcaaea/doc/hotspot-style.md#L1200-L1205
See also #24689
There was a problem hiding this comment.
Are you sure that none of the current usages of GrowableArray() relies on the initial capacity to be 2?
I've added a couple of suggestions for layout changes so that we can keep the structure of the constructors consistent. I think it makes it easier to see what this particular constructor is doing and how it differs from the other.
Also, previously, the constructors were order so that the CHeap version directly followed the Resource version:
GrowableArray() // Resource allocated
GrowableArray(int) // Resource allocated
GrowableArray(int, MemTag) // CHeap allocated
GrowableArray(int, int, const E&) // Resource allocated
GrowableArray(int, int, const E&, MemTag) // CHeap allocated
It would be nice to retain that structure. That is, just move the new GrowableArray(MemTag) up one step. Or, alternatively, completely separate the two groups of constructors into to sections.
| explicit GrowableArray(MemTag mem_tag) : | ||
| GrowableArrayWithAllocator<E, GrowableArray>(nullptr, 0), | ||
| _metadata(mem_tag) { | ||
| init_checks(); | ||
| } | ||
|
|
There was a problem hiding this comment.
| explicit GrowableArray(MemTag mem_tag) : | |
| GrowableArrayWithAllocator<E, GrowableArray>(nullptr, 0), | |
| _metadata(mem_tag) { | |
| init_checks(); | |
| } | |
| explicit GrowableArray(MemTag mem_tag) : | |
| GrowableArrayWithAllocator<E, GrowableArray>( | |
| nullptr, | |
| 0), | |
| _metadata(mem_tag) { | |
| init_checks(); | |
| } | |
| GrowableArray() : GrowableArrayWithAllocator<E, GrowableArray>(nullptr, 0), _metadata() { | ||
| init_checks(); | ||
| } |
There was a problem hiding this comment.
| GrowableArray() : GrowableArrayWithAllocator<E, GrowableArray>(nullptr, 0), _metadata() { | |
| init_checks(); | |
| } | |
| GrowableArray() : | |
| GrowableArrayWithAllocator<E, GrowableArray>( | |
| nullptr, | |
| 0), | |
| _metadata() { | |
| init_checks(); | |
| } |
| @@ -825,7 +833,9 @@ class GrowableArrayCHeap : public GrowableArrayWithAllocator<E, GrowableArrayCHe | |||
| } | |||
|
|
|||
| public: | |||
| GrowableArrayCHeap(int initial_capacity = 0) : | |||
| GrowableArrayCHeap() : GrowableArrayWithAllocator<E, GrowableArrayCHeap<E, MT>>(nullptr, 0) {} | |||
There was a problem hiding this comment.
| GrowableArrayCHeap() : GrowableArrayWithAllocator<E, GrowableArrayCHeap<E, MT>>(nullptr, 0) {} | |
| GrowableArrayCHeap() : | |
| GrowableArrayWithAllocator<E, GrowableArrayCHeap<E, MT>>( | |
| nullptr, | |
| 0) {} |
| @@ -825,7 +833,9 @@ class GrowableArrayCHeap : public GrowableArrayWithAllocator<E, GrowableArrayCHe | |||
| } | |||
|
|
|||
| public: | |||
| GrowableArrayCHeap(int initial_capacity = 0) : | |||
| GrowableArrayCHeap() : GrowableArrayWithAllocator<E, GrowableArrayCHeap<E, MT>>(nullptr, 0) {} | |||
There was a problem hiding this comment.
This change isn't necessary since the allocator doesn't allocate if you pass in 0. But maybe you want this change for clarity?
Hi,
This patch changes the default constructors of
GrowableArrayso that it does not allocate. This is helpful because sometimes we create aGrowableArrayand append another into it immediately, or create aGrowableArrayto merge the value from several branches. In these cases, the default allocation is not needed. This also aligns the behaviour with that ofstd::vector, which does not allocate for default construction.Please take a look and leave your reviews, thanks a lot.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24748/head:pull/24748$ git checkout pull/24748Update a local copy of the PR:
$ git checkout pull/24748$ git pull https://git.openjdk.org/jdk.git pull/24748/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24748View PR using the GUI difftool:
$ git pr show -t 24748Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24748.diff
Using Webrev
Link to Webrev Comment