Skip to content
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

Jump to bottom

Fix array constructor for Object Lists #17503

Merged
merged 1 commit into from
Jun 1, 2023
Merged

Fix array constructor for Object Lists #17503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 1 addition & 2 deletions runtime/gc_base/ContinuationObjectList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,8 @@ MM_ContinuationObjectList::newInstanceArray(MM_EnvironmentBase *env, uintptr_t a

continuationObjectLists = (MM_ContinuationObjectList *)env->getForge()->allocate(sizeof(MM_ContinuationObjectList) * arrayElements, MM_AllocationCategory::FIXED, J9_GET_CALLSITE());
if (NULL != continuationObjectLists) {
new(continuationObjectLists) MM_ContinuationObjectList[arrayElements]();

for (uintptr_t index = 0; index < arrayElements; index++) {
new(&continuationObjectLists[index]) MM_ContinuationObjectList();
Copy link
Contributor

Choose a reason for hiding this comment

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

Paretheses should not be used after the class name (and there should be a space after new):

			new (&continuationObjectLists[index]) MM_ContinuationObjectList;

Same comment applies to other files included in this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally like space between new and open bracket, but no-space is already very much adopted in J9 GC code (there are dozens of examples without space and barely any with).

As far as I know, the oval brackets are needed at the end - it's an indication that (in this case default) constructor should be invoked.

Copy link
Contributor

Choose a reason for hiding this comment

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

They are not needed; their presence represents a possible ambiguity.
Here we want the compiler to invoke the (user-defined) default constructor.
Using parentheses is a request for zero-initialization instead.
There's a discussion of this at https://stackoverflow.com/questions/620137/do-the-parentheses-after-the-type-name-make-a-difference-with-new.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reference to the discussion. But from what I can see, our case resembles to 'C' class (non-POD, with user defined ctor), and in both older/newer versions of compilers, calling it with '()', will call default ctor, what we want. I don't understand from where you get that it will do zero-init?

BTW, we have this pattern at a couple of 100 of spots, already. If we are indeed calling it wrong we are really lucky (that default ctor is just pretty much zero-init).

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps an analogy will help. You wouldn't write

    MM_ContinuationObjectList local();

to declare an initialize a local variable (the snippet declares a function which returns MM_ContinuationObjectList). Instead you would say

    MM_ContinuationObjectList local;

Along the same lines, I'm suggesting we should write new (where) T instead of new (where) T(). I believe there's potential to create ambiguity when a suitable call operator (operator()()) is available (although I haven't yet had luck constructing a concrete example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I somewhat buy that analogy and from that perspective, it would more consistent to do new (where) T instead of new (where) T(). But I don't see the reason to use the former to reduce/remove ambiguity. We probably always have default ctor (so it will be called over zero-init), and even if we don't I don't see a problem (it will call zero-init, what was likely the intention).

In presence of overloaded operator()(), yes, there could be ambiguity problems, but I don't want to go there. We have never overloaded it, and pretty sure never will. It's just such an extreme operator to overload.

So, do we want to go and change 10s of spots just for this analogy argument? I'd be ok if we are writing the code from scratch, but now, it's a tougher call. I'll think about it more...

Copy link
Contributor

Choose a reason for hiding this comment

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

It may not be worth fixing the existing instances of this, but I think it's something to keep in mind going forward: let's not make it worse.

continuationObjectLists[index].initialize(env);
}
}
Expand Down
3 changes: 1 addition & 2 deletions runtime/gc_base/OwnableSynchronizerObjectList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,8 @@ MM_OwnableSynchronizerObjectList::newInstanceArray(MM_EnvironmentBase *env, uint

ownableSynchronizerObjectLists = (MM_OwnableSynchronizerObjectList *)env->getForge()->allocate(sizeof(MM_OwnableSynchronizerObjectList) * arrayElements, MM_AllocationCategory::FIXED, J9_GET_CALLSITE());
if (NULL != ownableSynchronizerObjectLists) {
new(ownableSynchronizerObjectLists) MM_OwnableSynchronizerObjectList[arrayElements]();

for (uintptr_t index = 0; index < arrayElements; index++) {
new(&ownableSynchronizerObjectLists[index]) MM_OwnableSynchronizerObjectList();
ownableSynchronizerObjectLists[index].initialize(env);
}
}
Expand Down
3 changes: 1 addition & 2 deletions runtime/gc_base/ReferenceObjectList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,8 @@ MM_ReferenceObjectList::newInstanceArray(MM_EnvironmentBase *env, uintptr_t arra

referenceObjectLists = (MM_ReferenceObjectList *)env->getForge()->allocate(sizeof(MM_ReferenceObjectList) * arrayElements, MM_AllocationCategory::FIXED, J9_GET_CALLSITE());
if (NULL != referenceObjectLists) {
new(referenceObjectLists) MM_ReferenceObjectList[arrayElements]();

for (uintptr_t index = 0; index < arrayElements; index++) {
new(&referenceObjectLists[index]) MM_ReferenceObjectList();
referenceObjectLists[index].initialize(env);
}
}
Expand Down
3 changes: 1 addition & 2 deletions runtime/gc_base/UnfinalizedObjectList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@ MM_UnfinalizedObjectList::newInstanceArray(MM_EnvironmentBase *env, uintptr_t ar

unfinalizedObjectLists = (MM_UnfinalizedObjectList *)env->getForge()->allocate(sizeof(MM_UnfinalizedObjectList) * arrayElements, MM_AllocationCategory::FIXED, J9_GET_CALLSITE());
if (NULL != unfinalizedObjectLists) {
new(unfinalizedObjectLists) MM_UnfinalizedObjectList[arrayElements]();

for (uintptr_t index = 0; index < arrayElements; index++) {
new(&unfinalizedObjectLists[index]) MM_UnfinalizedObjectList();
unfinalizedObjectLists[index].initialize(env);
}
}
Expand Down