Skip to content

Conversation

@trquinn
Copy link
Collaborator

@trquinn trquinn commented Jul 2, 2025

In CkPupPerPlaceData(), numObjects will set _numGroups and _numNodeGroups, and these need to be greater than the maximum index of a valid entry in _groupTable and _nodeGroupTable, respectively.

This is a proposed fix for #3898 and #3678.
I will do some testing.

numObjects will set _numGroups and _numNodeGroups, and these need to be
greater than the maximum index of a valid entry in _groupTable and
_nodeGroupTable, respectively.
@trquinn
Copy link
Collaborator Author

trquinn commented Jul 4, 2025

I tested this on a run that hung after 5 to 8 restarts. This fix kept it going for at least 15 restarts.

Copy link
Contributor

@rbuch rbuch left a comment

Choose a reason for hiding this comment

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

Looks good to me, but, after looking at the other code a bit, it's unclear to me why the original code wasn't working. Do you have any insight on when gID.idx would be greater than numGroups? I see your other issues that indicate that this happens when ckDestroy is called, but I don't see any code that actually changes the size of the Group ID Table when a chare array is deleted. I don't think that should be a blocker to merging this in, I'm mostly just curious about it (but it's also been a couple years since I've looked at the Charm++ code base, so perhaps I'm missing something obvious here).

@rbuch rbuch requested review from ericjbohm and matthiasdiener July 6, 2025 16:00
Copy link
Contributor

@ericjbohm ericjbohm left a comment

Choose a reason for hiding this comment

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

Looks like it should address the problem.

@trquinn
Copy link
Collaborator Author

trquinn commented Jul 7, 2025

@rbuch If you delete a chare array and then create a new one, the ckNew() does not reuse groupIDs. In this case the largest groupID.idx will be greater than numGroups.

@ericjbohm ericjbohm merged commit 3cf60d5 into main Jul 7, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants