perf(config): simplify loadFromConfigPaths #1821
Conversation
Replace io.Reader-based config loading with direct []byte handling. Remove intermediate Merge wrapper, skip empty files. name old time/op new time/op delta LoadFromConfigPaths-12 52.4µs 53.5µs +2.0% name old alloc/op new alloc/op delta LoadFromConfigPaths-12 54.7KB 54.5KB -0.4% name old allocs/op new allocs/op delta LoadFromConfigPaths-12 477 472 -1.0% Assisted-by: Claude Opus 4.5 via Crush <crush@charm.land>
|
PS: this one would actually be way better allocation wise: #1285 |
There was a problem hiding this comment.
Pull request overview
This PR optimizes configuration loading by replacing io.Reader-based handling with direct []byte operations. The key changes simplify the code by eliminating the intermediate Merge wrapper function and preventing unnecessary file handle retention. Additionally, the PR introduces empty file skipping and adds comprehensive benchmark tests.
- Removed intermediate
Mergewrapper, usingjsons.Mergedirectly - Replaced
os.Openwithos.ReadFilefor simpler file handling without explicit Close calls - Added logic to skip empty configuration files
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/config/merge.go | Removed wrapper function that converted []io.Reader to []byte for jsons.Merge |
| internal/config/merge_test.go | Removed tests for the now-deleted Merge wrapper function |
| internal/config/load.go | Replaced io.Reader-based loading with []byte handling; removed LoadReader; added empty file skip; renamed loadFromReaders to loadFromBytes |
| internal/config/load_test.go | Updated test to use []byte instead of strings.NewReader; renamed test function |
| internal/config/attribution_migration_test.go | Updated test call to use []byte array instead of io.Reader |
| internal/config/load_bench_test.go | Added comprehensive benchmark tests for different config loading scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if os.IsNotExist(err) { | ||
| continue | ||
| } | ||
| return nil, fmt.Errorf("failed to open config file %s: %w", path, err) |
There was a problem hiding this comment.
The error message says "failed to open config file" but the code is now using os.ReadFile instead of os.Open. Consider updating the error message to "failed to read config file" for accuracy.
| return nil, fmt.Errorf("failed to open config file %s: %w", path, err) | |
| return nil, fmt.Errorf("failed to read config file %s: %w", path, err) |
| if len(data) == 0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The new behavior of skipping empty config files (line 630-632) lacks explicit test coverage. Consider adding a test case in load_test.go that verifies empty files are skipped during config loading, similar to the existing TestConfig_LoadFromBytes test.
| data, err := jsons.Merge(configs) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to merge configuration readers: %w", err) | ||
| return nil, err |
There was a problem hiding this comment.
The error from jsons.Merge is now returned without additional context. The previous code wrapped it with "failed to merge configuration readers". Consider adding context to help users understand what operation failed when they encounter this error.
| return LoadReader(merged) | ||
| var config Config | ||
| if err := json.Unmarshal(data, &config); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
The error from json.Unmarshal is returned without context. Consider wrapping it with additional information to help users understand that the merged configuration failed to parse, similar to how the previous error at line 628 provides context about which config file failed.
|
yes, #1285 gets rid of the json merge library - and also gives us better merges, with the other side of the coin being that we'll need to write more code (when adding new config fields, basically) |
@caarlos0 will merge this one in and probably merge the #1285 later, I need to either way change some stuff in config to support some workflow I want and I can include that PR there. |
Replace io.Reader-based config loading with direct []byte handling. Remove intermediate Merge wrapper, skip empty files.
basically,
go-jsonswould read all the readers entirely anyway, so this saves a couple of allocations and simplify things a bit. Also prevents leaving files opened for longer than necessary.time diff are negligible (noise), it does saves some allocations, the more files the merrier, but in the end it doesn't matter too much. Main thing is closing files early.