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

Improve support for JVM max heap configuration #65

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

hinerm
Copy link
Collaborator

@hinerm hinerm commented Dec 11, 2024

Adds necessary pre-reqs for fiji/fiji#351 by supporting .cfg overriding of a subset of JaunchConfig fields.

Also improves % expansion support in config.jvmMaxHeap specification.

@hinerm hinerm requested a review from ctrueden December 11, 2024 19:33
@ctrueden
Copy link
Member

This looks really good!

I'm going to test it a bit before merging. In particular, I want to verify how all this interacts with the .cfg files now... my thinking was that if the cfg file has e.g. max-heap = 5g then that should translate into cfg.max-heap in the vars list. But from the code as written, it's not clear to me (yet) how this actually works. So I'll study it!~

@hinerm
Copy link
Collaborator Author

hinerm commented Dec 11, 2024

if the cfg file has e.g. max-heap = 5g then that should translate into cfg.max-heap in the vars list.

I didn't do auto-prefixing like that because I didn't want to impact other settings in the cfg, e.g. for jvm.dir to become cfg.jvm.dir. Yes, you could do . checking but why only one depth? It felt arbitrary and there aren't enough settings currently in the .cfg to create a pattern for abstraction

Also, you have to refer to it as cfg.max-heap in the .toml, so I liked the clarity in the .cfg that "this is a setting that is overriding a toml property". I prefer not having magic that leads to the settings referenced differently in different locations.

JaunchConfig fields prefixed with "cfg." will now explicitly allow
override via reading .cfg files. This allows us to set default field
values in the .toml but later allow the user to specify these values in
a more-easily modified config file.

The general pattern is illustrated here with the jvmMaxHeap field:
- a cfg.max-heap field is delcared in the toml with a default value
- jvm.max-heap is defined in the toml with a value of cfg.max-heap
- when jvm.max-heap is interpolated it will resolve to the current
  cfg.max-heap value, which may have been overridden by a .cfg file

Required for fiji/fiji#351
@ctrueden ctrueden self-assigned this Dec 11, 2024
Previously we were performing special-case parameter processing (e.g.
expanding %'s in jvmMaxHeap specifications) at the time of initial
runtime configuration. However, this would happen a) before variable
interpolation and b) only if the user hadn't manually specified a value.
As a result, %'s would only work in a very limited number of cases (when
set in the .toml)

Instead, we now do a second "processArgs" pass that happens after
variable interpolation. It is intended to operate on all runtime args at
that point, whether they came from a toml configuration, user, etc...

This allows the particular case of % calculation in jvmMaxHeap to
trigger in all cases.

Closes #64.

Note that because we process every item in the arg list we are working
around #63, without actually fixing it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants