Raise number of threads used by GASNet by default + add warning if exceeded #26448
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ceeded --- Signed-off-by: Brad Chamberlain <[email protected]>
bonachea
reviewed
Dec 21, 2024
Borrowed from @bonachea's PR in chapel-lang#25414 which contained this along with the other Makefile change I made in this PR. --- Signed-off-by: Brad Chamberlain <[email protected]>
jabraham17
approved these changes
Jan 15, 2025
bonachea
approved these changes
Jan 15, 2025
This does two things: * specializes the error message I just added to only print the GASNet-specific information if CHPL_COMM is GASNet once Jade reminded me of how easy it was * fixes an issue that I noticed in testing where the value being returned was the hard limit on the number of threads that GASNet is configured to use rather than the soft limit that it's using in practice. With Dan's help, I updated this to the API that we should've been using all along and put in some bounds-checking code that is unlikely to ever matter (b/c we configure the cap at well within an int(32)'s value and b/c it's hard to imagine a user setting it higher than max(int(32)). --- Signed-off-by: Brad Chamberlain <[email protected]>
jabraham17
approved these changes
Jan 16, 2025
bonachea
suggested changes
Jan 16, 2025
…this out) --- Signed-off-by: Brad Chamberlain <[email protected]>
This test formerly tried to exceed our thread limits by setting the number of threads to 300, but that limit is now below the GASNet default soft limit of 1024, so here, I bumped the number of threads up to 2000 to exceed that limit again, and keep it testing the same things. --- Signed-off-by: Brad Chamberlain <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[co-developed by @bonachea]
As noted in #26379, we've recently had a user on a high-core-count system get surprised when we silently dialed the maximum number of tasks down to 255 rather than using the number of cores on that system. This PR addresses that issue in two ways:
GASNET_MAX_THREADSIn testing this, I also updated the
chpl_comm_getMaxThreads()call to use the soft limit from GASNet on the number of threads rather than the hard, configuration-time limit that we were using previously. Since these two values have traditionally been the same (by default at least, assuming the user didn't override either), this has not been an issue, but now that we're configuring to one value and relying on GASNet's default for the other, it becomes more iimportant. Without this change, the implication is that if the user had used more than 1024 threads, they could exceed the size of the arrays that GASNet had allocated and bad things would happen.While here, I also noticed that in our Makefiles we seemed to be passing the list of GASNet configuration options to it twice, introduced in 2014 in d765982. I don't see any indication that that's anything more intentional than a typo, so here, I've removed one of the occurrences, leaving the one at the end of the line there, thinking it'd support overriding other, earlier options. After the fact, Dan pointed out that he'd made the same change in #25414 — a PR that seems to have fallen between the cracks at some point. I also adopted the
FORCEis a phony target rule from there to permit it to be closed and opened #26548 to note that we should do the same in other Makefiles.Resolves #26379
Resolves #25414