search

chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.76k stars 414 forks source link

Should we have a global `CHPL_MEM` and `CHPL_JEMALLOC`? #25359

Open jabraham17 opened 1 month ago

jabraham17 commented 1 month ago

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

The reason for the duplicate names CHPL_MEM/CHPL_TARGET_MEM and CHPL_JEMALLOC/CHPL_TARGET_JEMALLOC is to provide backwards compatibility after https://github.com/chapel-lang/chapel/pull/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 https://github.com/chapel-lang/chapel/issues/25347#issuecomment-2187680628 for more details on how these defaults would work.

bradcray commented 1 month ago

I like the idea of having CHPL_MEM or CHPLJEMALLOC provide the defaults for CHPLMEM/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 commented 1 month ago

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 commented 1 month ago

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?