chapel-lang / chapel

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

Make CHPL*_JEMALLOC setting(s) first-class / documented? #25347

Open bradcray opened 2 weeks ago

bradcray commented 2 weeks ago

While reviewing CHANGES.md entries for 2.1, I found myself wondering why CHPL_TARGET_JEMALLOC was mentioned in the developer-only section rather than the user-facing section. Turns out it's not/has never been user-facing, nor has/wasCHPL_JEMALLOCthough it seems that CHPL_HOST_JEMALLOC is.

With a quick check with @jabraham17 and @mppf on #6133 and Slack, none of us seemed to have reasons that it wouldn't be treated symmetrically with other CHPL_* settings as things stand today, so this issue captures the desire to do so. I think of this as meaning that the docs would mention the variable(s) alongside others and printchplenv would print it/them out (presumably only when CHPL_MEM=jemalloc). (My confusion over whether to use the singular or plural in the previous sentence is due to my uncertainty around whether we're still supporting / wanting to support CHPL_JEMALLOC).

jabraham17 commented 2 weeks ago

I've opened https://github.com/chapel-lang/chapel/pull/25352 to implement this. There is some followup here that I noticed while doing this.

As Brad mentioned in the OP, we still have CHPL_JEMALLOC, which is a synonym for CHPL_TARGET_JEMALLOC. If both are specified, CHPL_TARGET_JEMALLOC is preferred and a warning is emitted. This is also true for CHPL_MEM and CHPL_HOST_MEM. However, no warning is emitted if just CHPL_JEMALLOC (or just CHPL_MEM) is used.

We have had these warnings for several years now and I think we should remove them (with a deprecation warning first).

bradcray commented 2 weeks ago

If I set CHPL_JEMALLOC, do both CHPL_HOST_JEMALLOC and CHPL_TARGET_JEMALLOC take their values from it? If so, I could see retaining the current behavior as a way to set just a single thing rather than two when a user wants them to be the same and not the default. I just wouldn't describe it as a synonym, but as a way to select which jemalloc is used for both host and target as a convenience.

This is also true for CHPL_MEM and CHPL_HOST_MEM.

I have the similar question here w.r.t. the relationship between CHPL_MEM, CHPL_HOST_MEM, and CHPL_TARGET_MEM

jabraham17 commented 2 weeks ago

No, CHPL_JEMALLOC only affects CHPL_TARGET_JEMALLOC. Same with CHPL_MEM, its only related to CHPL_TARGET_MEM.

If I understand the history right, this stems from https://github.com/chapel-lang/chapel/pull/18627, which added CHPL_HOST_MEM. So we used to not distinguish between host and target (host always used cstdlib), and then added support for building the compiler with jemalloc. And essentially CHPL_JEMALLOC is the old name and CHPL_TARGET_JEMALLOC and CHPL_HOST_JEMALLOC are the two new replacement names. Same with CHPL_MEM

bradcray commented 2 weeks ago

Do you think there's a path to having the behavior I'm suggesting without breaking peoples' existing configurations? (at least, silently?). Specifically, am I correct that if I wanted to use a system version of jemalloc for both host and target, I'd need to do:

CHPL_HOST_JEMALLOC=system
CHPL_TARGET_JEMALLOC=system
# would I have to do any CHPL_*MEM settings as well?  I can't recall what the HOST case defaults to…

Probably this is already abundantly clear, but it seems nice if I could just do:

CHPL_JEMALLOC=system

and get the same behavior as above (one shorter setting rather than two longer ones). Similarly if I wanted to do CHPL_MEM=cstdlib for both host and target (assuming they currently have the same default…?)

jabraham17 commented 2 weeks ago

I think we could probably change the meaning of CHPL_JEMALLOC to mean "use this for TARGET and HOST" and similarly change the meaning of CHPL_MEM to "use this for TARGET and HOST" silently and have no issue.

For example, a user has the following

CHPL_HOST_JEMALLOC=system
CHPL_JEMALLOC=bundled

Then I would interpret this as "use bundled jemalloc for both TARGET and HOST", and then use the value supplied to CHPL_HOST_JEMALLOC to override the "default".

However, this brings up the question of what is the default? CHPL_TARGET_JEMALLOC currently defaults to bundled and CHPL_HOST_JEMALLOC has a default dependent on the system. So whats the default of CHPL_JEMALLOC?

The same problem occurs with CHPL_MEM, where CHPL_TARGET_MEM defaults to jemalloc (except for cygwin) and CHPL_HOST_MEM has a default dependent on the system

Personally, I would rather get rid of CHPL_MEM and CHPL_JEMALLOC then do the "make this the default" approach, because I think this adds another level of complexity both for users and for the build scripts. But if others think differently I think we could make it work without breaking user code

bradcray commented 2 weeks ago

Then I would interpret this as "use bundled jemalloc for both TARGET and HOST", and then use the value supplied to CHPL_HOST_JEMALLOC to override the "default".

I suppose that's true. I was thinking of the warnings we generate today, but in your approach, they would go away, which makes sense to me. Or perhaps they should be preserved if all three are set (like "you either don't know all these are set, or there's some mistake in your mental model").

However, this brings up the question of what is the default? CHPL_TARGET_JEMALLOC currently defaults to bundled and CHPL_HOST_JEMALLOC has a default dependent on the system. So whats the default of CHPL_JEMALLOC?

Does CHPL_JEMALLOC actually need a deafult? That is, I'm imagining:

This is admittedly a little more complex for us to support, but it seems to simplify the user's experience.

Seems like we might want to fork this off to its own issue in any case since it's probably independent of simply exposing what we have today?

bradcray commented 2 weeks ago

Thinking about the logic above in terms of our normal default tables, I'm imagining it'd be something like:

CHPL_JEMALLOC: has no default

CHPL_HOST_JEMALLOC: defaults to `CHPL_JEMALLOC` if set, otherwise to..
[existing table here]

CHPL_TARGET_JEMALLOC: defaults to `CHPL_JEMALLOC` if set, otherwise to `bundled`

and similarly for CHPL_*MEM.

jabraham17 commented 2 weeks ago

I have spun out a separate issue for this here