Skip to content

8355013: GrowableArray default constructor should not allocate #24748

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

8355013: GrowableArray default constructor should not allocate #24748

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af6f640
non-allocating constructors for GrowableArray
merykitty Apr 18, 2025
118ab35
refine tests
merykitty Apr 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions src/hotspot/share/utilities/growableArray.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ void GrowableArrayWithAllocator<E, Derived>::expand_to(int new_capacity) {
template <typename E, typename Derived>
void GrowableArrayWithAllocator<E, Derived>::grow(int j) {
// grow the array by increasing _capacity to the first power of two larger than the size we need
expand_to(next_power_of_2(j));
expand_to(next_power_of_2(MAX2(j, 4)));
}

template <typename E, typename Derived>
Expand Down Expand Up @@ -754,7 +754,9 @@ class GrowableArray : public GrowableArrayWithAllocator<E, GrowableArray<E>> {
}

public:
GrowableArray() : GrowableArray(2 /* initial_capacity */) {}
GrowableArray() : GrowableArrayWithAllocator<E, GrowableArray>(nullptr, 0), _metadata() {
init_checks();
}
Comment on lines +757 to +759
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
GrowableArray() : GrowableArrayWithAllocator<E, GrowableArray>(nullptr, 0), _metadata() {
init_checks();
}
GrowableArray() :
GrowableArrayWithAllocator<E, GrowableArray>(
nullptr,
0),
_metadata() {
init_checks();
}


explicit GrowableArray(int initial_capacity) :
GrowableArrayWithAllocator<E, GrowableArray>(
Expand All @@ -764,6 +766,12 @@ class GrowableArray : public GrowableArrayWithAllocator<E, GrowableArray<E>> {
init_checks();
}

explicit GrowableArray(MemTag mem_tag) :
GrowableArrayWithAllocator<E, GrowableArray>(nullptr, 0),
_metadata(mem_tag) {
init_checks();
}

Comment on lines +769 to +774
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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(int initial_capacity, MemTag mem_tag) :
GrowableArrayWithAllocator<E, GrowableArray>(
allocate(initial_capacity, mem_tag),
Expand Down Expand Up @@ -825,7 +833,9 @@ class GrowableArrayCHeap : public GrowableArrayWithAllocator<E, GrowableArrayCHe
}

public:
GrowableArrayCHeap(int initial_capacity = 0) :
GrowableArrayCHeap() : GrowableArrayWithAllocator<E, GrowableArrayCHeap<E, MT>>(nullptr, 0) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
GrowableArrayCHeap() : GrowableArrayWithAllocator<E, GrowableArrayCHeap<E, MT>>(nullptr, 0) {}
GrowableArrayCHeap() :
GrowableArrayWithAllocator<E, GrowableArrayCHeap<E, MT>>(
nullptr,
0) {}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change isn't necessary since the allocator doesn't allocate if you pass in 0. But maybe you want this change for clarity?


explicit GrowableArrayCHeap(int initial_capacity) :
GrowableArrayWithAllocator<E, GrowableArrayCHeap<E, MT> >(
allocate(initial_capacity, MT),
initial_capacity) {}
Expand Down
14 changes: 12 additions & 2 deletions test/hotspot/gtest/utilities/test_growableArray.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,12 @@ class GrowableArrayTest : public ::testing::Test {
// Combination not supported

// Stack/Resource allocated
{
ResourceMark rm;
GrowableArray<int> a;
modify_and_test(&a, modify, test);
}

{
ResourceMark rm;
GrowableArray<int> a(max);
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are these used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


TEST_VM_F(GrowableArrayTest, append) {
with_all_types_all_0(Append);
}
Expand Down Expand Up @@ -561,7 +571,7 @@ TEST_VM_ASSERT_MSG(GrowableArrayAssertingTest, assignment_with_embedded_cheap,
TEST(GrowableArrayCHeap, sanity) {
// Stack/CHeap
{
GrowableArrayCHeap<int, mtTest> a(0);
GrowableArrayCHeap<int, mtTest> a;
#ifdef ASSERT
ASSERT_TRUE(a.allocated_on_stack_or_embedded());
#endif
Expand All @@ -574,7 +584,7 @@ TEST(GrowableArrayCHeap, sanity) {

// CHeap/CHeap
{
GrowableArrayCHeap<int, mtTest>* a = new GrowableArrayCHeap<int, mtTest>(0);
GrowableArrayCHeap<int, mtTest>* a = new GrowableArrayCHeap<int, mtTest>();
#ifdef ASSERT
ASSERT_TRUE(a->allocated_on_C_heap());
#endif
Expand Down