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

Raise number of threads used by GASNet by default + add warning if exceeded #26448

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Raise number of threads used by GASNet by default + add warning if exceeded #26448

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
12 changes: 11 additions & 1 deletion runtime/src/tasks/qthreads/tasks-qthreads.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,17 @@ static void setupAvailableParallelism(int32_t maxThreads) {
if (hwpar > 0) {
// Limit the parallelism to the maximum imposed by the comm layer.
if (0 < maxThreads && maxThreads < hwpar) {
hwpar = maxThreads;
if (chpl_nodeID == 0) {
char msg[1024];
snprintf(msg, sizeof(msg),
"The CHPL_COMM setting is limiting the number of threads "
"to %d, rather than the hardware's preference of %d. "
"If using CHPL_COMM=gasnet, set 'GASNET_MAX_THREADS' to "
"a higher number in your environment to address this.\n",
maxThreads, hwpar);
chpl_warning(msg, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

New warning text LGTM, and should usually achieve the goal of avoiding silent under-utilization of cores.

However IIUC the new warning won't be printed on a heterogeneous system where node 0 has a smaller core count than maxThreads, but some other node might have a higher core count.

Not sure if such node heterogeneity is of interest (or even permitted by Chapel?) so feel free to ignore this observation if it seems irrelevant or not worth addressing.

}
hwpar = maxThreads;
}

//
Expand Down
2 changes: 1 addition & 1 deletion third-party/gasnet/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ gasnet-config: FORCE
mkdir -p $(GASNET_BUILD_DIR)
cd $(GASNET_SUBDIR) && ./Bootstrap -T
$(XTRA_CONFIG_COMMAND)
$(ENSURE_GASNET_COMPATIBLE_COMPILER) cd $(GASNET_BUILD_DIR) && $(CHPL_GASNET_ENV_VARS) $(GASNET_SUBDIR)/$(CHPL_GASNET_CFG_SCRIPT) --prefix=$(GASNET_INSTALL_DIR) $(CHPL_GASNET_CFG_OPTIONS) --disable-seq --enable-par --disable-parsync $(CHPL_GASNET_CFG_OPTIONS)
$(ENSURE_GASNET_COMPATIBLE_COMPILER) cd $(GASNET_BUILD_DIR) && $(CHPL_GASNET_ENV_VARS) $(GASNET_SUBDIR)/$(CHPL_GASNET_CFG_SCRIPT) --prefix=$(GASNET_INSTALL_DIR) --disable-seq --enable-par --disable-parsync --with-max-pthreads-per-node=65535 $(CHPL_GASNET_CFG_OPTIONS)
Copy link
Contributor

Choose a reason for hiding this comment

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

This hunk partially overlaps the same fix proposed in the first commit of #25414 for the same cosmetic defect.

It sounds like we both independently made the same observation about the GASNet configure line and arrived at the same fix, so feel free to close #25414 in favor of this PR.


gasnet-build: FORCE
$(ENSURE_GASNET_COMPATIBLE_COMPILER) cd $(GASNET_BUILD_DIR) && $(MAKE) all
Expand Down
Loading