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.

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

Should we have a global CHPL_MEM and CHPL_JEMALLOC? #25359

Open
jabraham17 opened this issue Jun 25, 2024 · 3 comments
Open

Should we have a global CHPL_MEM and CHPL_JEMALLOC? #25359

jabraham17 opened this issue Jun 25, 2024 · 3 comments

Comments

@jabraham17
Copy link
Member

Currently, we have the following environment variables for configuring Chapels memory allocation

  • CHPL_MEM: the allocator to use for compiled chapel programs. Either cstdlib or jemalloc
    • CHPL_TARGET_MEM: same as CHPL_MEM, specifying both gives a warning and takes the value from CHPL_TARGET_MEM
  • CHPL_HOST_MEM: the allocator to use for the chapel compiler. Either cstdlib or jemalloc
  • CHPL_JEMALLOC: when CHPL_MEM/CHPL_TARGET_MEM is set to jemalloc, switches between system and jemalloc
    • CHPL_TARGET_JEMALLOC: same as CHPL_JEMALLOC, specifying both gives a warning and takes the value from CHPL_TARGET_JEMALLOC
  • CHPL_HOST_JEMALLOC: when CHPL_HOST_MEM is set to jemalloc, switches between system and jemalloc

The reason for the duplicate names CHPL_MEM/CHPL_TARGET_MEM and CHPL_JEMALLOC/CHPL_TARGET_JEMALLOC is to provide backwards compatibility after #18627. Note that CHPL_JEMALLOC is not a user facing feature and CHPL_MEM has been documented to be deprecated in favor of CHPL_TARGET_MEM since that PR.

Do we still want to support CHPL_MEM/CHPL_JEMALLOC?

The simple thing to do is to just remove CHPL_MEM and CHPL_JEMALLOC, however another possibility brought up on #25347 is to keep these variables, but just change their behavior slightly. CHPL_MEM would specify the default for CHPL_TARGET_MEM and CHPL_HOST_MEM, such that if CHPL_MEM is set to jemalloc then both TARGET and HOST are jemalloc. And similarly for CHPL_JEMALLOC. This would provide a shortcut for specifying the host and target memory configuration for users.

See #25347 (comment) for more details on how these defaults would work.

@bradcray
Copy link
Member

bradcray commented Jun 25, 2024

I like the idea of having CHPL_MEM or CHPL_JEMALLOC provide the defaults for CHPL_MEM/CHPL_JEMALLOC respectively, but want to note that it would arguably be a non-breaking change to go with the simple thing for now and to introduce the umbrella variables if/when users complain about having to set multiple things to use a given jemalloc for both host and target (does that sound right, Jade?). So maybe that's what we should do—pursue the simple cleanup for now to deprecate the somewhat outdated redundancy and create (or repurpose) this issue to capture the notion of the umbrella variables to see whether it garners support over time, or simply remains "a nice idea, but not crucial".

[edit: I guess arguably this issue already serves the purpose without a lot of refactoring. I was viewing it as a "should we do this or that?" issue, but re-reading the title, I suppose it really is more of a "Should we do that?" issue. :) ]

@jabraham17
Copy link
Member Author

I agree that it would be a non-breaking change to do the simple thing for now and add the umbrella variables later if-needed. This would be my preference.

I do think that if we take that route, before we remove CHPL_JEMALLOC and CHPL_MEM, we should have warnings if these exist in an users environment at all, telling users to switch to the undeprecated terms. Just because our docs say CHPL_MEM is deprecated doesn't mean users have read that :).

@bradcray
Copy link
Member

I agree, and was imagining printchplenv (or something in the build process) print the warning rather than just relying on docs in doing the deprecation. Why don't we proceed with that plan if nobody speaks up organically on this issue in the meantime?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants