Repository navigation
Conversation
Settings becomes a tree of records: each nested record is a section of config.yml and each component an option, with its comment in @comment and its default in @default. ConfigMapper binds a parsed file to the records and writes them back, so key names no longer come from field names by reflection. config.yml keeps its keys, order, comments and defaults. - Settings.override and Settings#with change an option through an accessor reference, so a wrong value type is a compile error. - Reloading builds a new tree and swaps one reference. - Strings are escaped when written, an unreadable file is not overwritten, and an unchanged file is not rewritten. - LimitResolver prepares each limit group once per loaded configuration instead of on every getLimit call. - PROTOCOL_SUPPORT_FIX moves to FaweCache#setProtocolSupportFix, since a record cannot hold a runtime flag. Config and ConfigOptComputation are removed and every use of the old Settings fields is migrated.
Settings becomes a tree of records: each nested record is a section of config.yml and each component an option, with its comment in @comment and its default in @default. ConfigMapper binds a parsed file to the records and writes them back, so key names no longer come from field names by reflection. config.yml keeps its keys, order, comments and defaults. - Settings.override and Settings#with change an option through an accessor reference, so a wrong value type is a compile error. - Reloading builds a new tree and swaps one reference. - Strings are escaped when written, an unreadable file is not overwritten, and an unchanged file is not rewritten. - LimitResolver prepares each limit group once per loaded configuration instead of on every getLimit call. - PROTOCOL_SUPPORT_FIX moves to FaweCache#setProtocolSupportFix, since a record cannot hold a runtime flag. Config and ConfigOptComputation are removed and every use of the old Settings fields is migrated.
6ef49ee to
4adf724
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It is an explicitly Work-In-Progress draft that rewrites the core configuration system and mechanically touches ~70 call sites, which warrants human review before approval.
3 open issues
What changed in this PR
This PR replaces FAWE's legacy reflection-based Config.java configuration system with an immutable, record-based Settings API. Configuration sections become nested Java records whose components are annotated with @Comment/@Default, and a new mapping layer (ConfigMapper, ConfigYaml, ConfigTree, LimitResolver) handles record↔YAML serialization, migration of legacy keys, and limit-group resolution. Roughly 70 call sites are mechanically migrated from uppercase field access (Settings.settings().QUEUE.PARALLEL_THREADS) to accessor methods (Settings.settings().queue().parallelThreads()). The PR is explicitly a Work-In-Progress draft.
Changes:
- Introduce an immutable record-based
Settingsmodel plus a new serialization/migration framework and comprehensive unit tests. - Rewrite
Fawe.setupConfigs()to load/save viaSettings.load/savewith aBuildInfo, and move theFaweCache.protocolSupportFixflag off the global settings object. - Migrate ~70 call sites to the new accessor API and delete
Config.java/ConfigOptComputation.java.
| File | Description |
|---|---|
.../configuration/Settings.java |
New immutable record config model, load/save/migrate/defaults and BuildInfo (context only; not in diff). |
.../configuration/ConfigMapper.java |
Reflection-based record↔YAML mapping, coercion, and defaults. |
.../configuration/ConfigYaml.java |
Hand-rolled UTF-8 YAML reader/writer with quoting/escaping. |
.../configuration/ConfigTree.java |
Dotted-path tree used by migration. |
.../configuration/LimitResolver.java |
Merges limit groups into a resolved FaweLimit ("most generous" semantics). |
.../configuration/{Option,Default,Comment}.java |
Annotations/metadata driving option paths, defaults and comments. |
.../core/Fawe.java |
Rewritten setupConfigs() using Path, Settings.load/save, and BuildInfo. |
.../core/FaweCache.java |
New protocolSupportFix field + setter replacing the old global setting. |
.../test/.../SettingsTest.java, ConfigMapperTest.java |
New unit tests covering save/load, migration, limits and fuzzed text round-tripping. |
.../test/resources/.../config.yml |
Expected-output fixture mirroring the @Comment/@Default text; carries over several comment typos. |
| ~70 migrated call sites | Mechanical switch from uppercase fields to accessor methods. |
Review effort: Balanced
…to refactor/v3/settings-records
…rate schematic limits into default Also makes protocolSupportFix volatile and sets the remaining @SInCE tags to 3.0.0.
…n configuration handling
… into refactor/v3/settings-records
…form from the saved config Also finishes the option to setting rename in javadoc and replaces the assert with Entry.requireType().
…ngs-records Brings in main via v3, including 26.3 support. Config.java stays deleted and Settings.java keeps the records version: ConfigYaml already writes no space after a list key and the comment strings carry no trailing spaces, which is what #3675 and #3676 fixed in the old classes. The 26.3 adapter gets the same record accessors as the other adapters.
- Remove the unused experimental.anvil-queue-mode setting and a duplicated history comment line; existing files have the key reported and removed. - Use var for entry loops and fix minor javadoc in the configuration classes. - Remove savedSettingsLoadBackUnchanged and missingFileGivesDefaults, which repeated anyTextIsReadBackAsWritten and aFileWithNothingToReadGivesTheFallback. - Make the allowed-image-hosts case differ from its default so it can fail, and drop input lines no assertion checked. - Add YAML 1.1 boolean words to the anyTextIsReadBackAsWritten samples.
Co-authored-by: MattBDev <4009945+MattBDev@users.noreply.github.com>
Co-authored-by: MattBDev <4009945+MattBDev@users.noreply.github.com>
|
Marking the PR as ready for review but I still have to update the PR body. Should be done Saturday. I think I have a minor tweak or two to I have to push as well. |
There was a problem hiding this comment.
🟡 Changes recommended
An unresolved critical finding remains: a typo in the config.yml comment (clipboad.on-disk instead of clipboard.use-disk) that must be mirrored in the Settings.java @Comment or the defaultsAreWrittenAsExpected test will fail.
1 open issue
3 resolved since last review
Review effort: Balanced
Co-authored-by: MattBDev <4009945+MattBDev@users.noreply.github.com>
SirYwell
left a comment
SirYwell
left a comment
There was a problem hiding this comment.
What is the future updating strategy with this? E.g., if we want to rename entries etc.
| * write directly. Going through SnakeYAML's node tree and emitter made saving slower than the old hand-written output. | ||
| * The bytes are compared with the file and written from the same buffer, so the content is never copied. | ||
| */ | ||
| static final class Writer { |
There was a problem hiding this comment.
What's the purpose of this? Why can't we use snakeyaml here? It looks far too hacky to be maintainable.
There was a problem hiding this comment.
I tried it. I swapped in SnakeYAML's writer and ran the same tests and benchmarks against it.
SnakeYAML can write the config.yml, but it's a lot slower. Building the file takes ~8.5x as long, a save takes somewhere between 2-3x as long, and it uses as much as 6x more memory per save.
While SnakeYAML has to handle any YAML file, ours only has to handle this one. It turns the settings into a tree of objects, then into a second set of event objects, checks every value against its type patterns and works out how to quote it, things we don't have to worry about. The file is in memory three times before it's written. This writer goes from settings to bytes, avoiding the memory overhead by encoding into one buffer and writing that buffer to the file.
While maintaining our own writer is an added burden, we benefit from added speed and memory savings. This writer aligns with the plugins goal of minimizing memory usage and improves performance where possible.
Besides, we have been writing the config by hand for a while. Copilot running Claude did a search and it found we've been doing this since 2018 with Config.toYamlString(). That had its own problems of not escaping everything correctly.
I can change the writer so it's a bit smaller and use StringBuilder but would come with a small memory increase. I'm experimenting with a few other alternatives, but I am satisfied with this writer as it stands which is why I sent the PR.
There was a problem hiding this comment.
I don't think we should look at performance too much here. But all the magic values in there with the dirty details of YAML are extremely scary. SnakeYAML has a direct and indirect test coverage that is a whole different world from whatever we can achieve. I think this isn't the last place we need to look for highly customized solutions in FAWE.
There was a problem hiding this comment.
Okay. I'll replace it with SnakeYAML. It will lose a lot of the memory and performance benefits but the config system will still look nicer.
Migration is handled in |
| ) throws IOException { | ||
| actor.print(TextComponent.of("Please wait while we generate the minified heightmaps.")); | ||
| File srcFolder = MainUtil.getFile(Fawe.platform().getDirectory(), Settings.settings().PATHS.HEIGHTMAP); | ||
| File srcFolder = MainUtil.getFile(Fawe.platform().getDirectory(), Settings.settings().paths().heightmap()); |
There was a problem hiding this comment.
Maybe a follow-up change/stacked pr: replace File with Path?
There was a problem hiding this comment.
It's in the works ;)
|
|
||
| // ProtocolSupport cannot read sections that use fewer than 4 bits per entry. Volatile as it is set on the main thread | ||
| // and read by the threads packing chunk sections | ||
| private volatile boolean protocolSupportFix; |
There was a problem hiding this comment.
Not happy with this. I don't think it needs to be volatile given how this should always happen-before any edit is started, right? In best case we could make it final static, but that would require an additional class...
There was a problem hiding this comment.
I'll double check this to make sure. thanks for pointing that out.
| private static final List<String> INFORMATION = List.of("issues", "wiki"); | ||
|
|
||
| // Not volatile: settings are immutable, so a thread that has not yet seen a reload reads the previous ones, whole | ||
| private static Settings instance = defaults(); |
There was a problem hiding this comment.
This is a place where I think being more experimental makes sense. We should try and see if we can use MutableCallSite in a static final field here and read from that. That would allow fully constant-folding all the values through the records.
There was a problem hiding this comment.
Not familiar with this class but I'll look into it.
Description
FAWE's
config.ymlis now loaded into read-only Java records. Each setting's default is written next to the setting, and code changes a setting with a method reference such asSettings.History::compressionLevel.Removed API
Settings.settings().QUEUE.PARALLEL_THREADSSettings.settings().queue().parallelThreads()Settings.IMPSettings.settings()Settings.settings().EXPERIMENTAL.PERSISTENT_BRUSHES = falseSettings.override(Settings.Experimental::persistentBrushes, false)Settings.settings().reload(File)Settings.set(Settings.load(Path))Settings.settings().save(File)settings.save(Path)Settings.settings().LIMITS.get("default")Settings.settings().defaultLimits()Settings.settings().PROTOCOL_SUPPORT_FIXFaweCache.INSTANCE.setProtocolSupportFix(boolean)Configand its annotations@Comment,@DefaultPerformance
The new code loads, reloads and saves faster, works out limits more than twice as fast, and allocates much less memory. Reading a setting costs the same.
getLimit, player in no group, stock filegetLimit, player in two groups, 8 groups