Skip to content

Commit a7b0ccf

Browse files
authored
Refactor migrations to only marshall yaml twice (#4318)
- **PR Description** In the solving of #4194 we had some discussion about the repeated marshalling and unmarshalling of the content in our migrations. #4194 (comment). I was feeling the itch to see if there was a meaningful performance impact to that, and this is the results! I added a benchmark first that uses the entirety of `Config.md`. This ensures a worst-case scenario for the repeated parsing, and probably at least 1 user has done this for their config. Here were the benchmark results prior and after this PR: ## Before ``` $ go test ./pkg/config/ -bench=. -benchmem -count=10 goos: linux goarch: amd64 pkg: github.com/jesseduffield/lazygit/pkg/config cpu: Intel(R) Core(TM) i7-8705G CPU @ 3.10GHz BenchmarkMigrationOnLargeConfiguration-8 85 18165916 ns/op 1501442 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 50 21024442 ns/op 1501473 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 75 18814769 ns/op 1501509 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 60 17886812 ns/op 1501434 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 97 17767498 ns/op 1501448 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 62 18749923 ns/op 1501478 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 62 19248265 ns/op 1501467 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 92 14771199 ns/op 1501423 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 67 16345152 ns/op 1501440 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 98 16785737 ns/op 1501472 B/op 19316 allocs/op PASS ok github.com/jesseduffield/lazygit/pkg/config 14.474s ``` so somewhere in the range of 14 to 21 ms added to startup time. ## After ``` $ go test ./pkg/config/ -bench=. -benchmem -count=10 goos: linux goarch: amd64 pkg: github.com/jesseduffield/lazygit/pkg/config cpu: Intel(R) Core(TM) i7-8705G CPU @ 3.10GHz BenchmarkMigrationOnLargeConfiguration-8 440 2936145 ns/op 265800 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 428 3099183 ns/op 265787 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 355 3074069 ns/op 265785 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 478 3031144 ns/op 265792 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 375 2795470 ns/op 265785 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 406 2688491 ns/op 265791 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 375 3092990 ns/op 265791 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 379 2789022 ns/op 265785 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 417 3108530 ns/op 265785 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 487 2848502 ns/op 265788 B/op 3928 allocs/op PASS ok github.com/jesseduffield/lazygit/pkg/config 15.352s ``` So somewhere between 2.6 and 3.1 ms. Is that a meaningful speedup? I'm not sure, but it's something! I had to add the bit of complexity mentioned in the comment to track whether we had made any changes, but that doesn't feel fundamentally much more complex to me, since we were already implicitly tracking that fact with whether the string file changed. - **Please check if the PR fulfills these requirements** * [X] Cheatsheets are up-to-date (run `go generate ./...`) * [X] Code has been formatted (see [here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#code-formatting)) * [ ] Tests have been added/updated (see [here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md) for the integration test guide) * [ ] Text is internationalised (see [here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation)) * [ ] If a new UserConfig entry was added, make sure it can be hot-reloaded (see [here](https://github.com/jesseduffield/lazygit/blob/master/docs/dev/Codebase_Guide.md#using-userconfig)) * [ ] Docs have been updated if necessary * [X] You've read through your own file changes for silly mistakes etc
2 parents 4e38a94 + d39f883 commit a7b0ccf

File tree

4 files changed

+745
-150
lines changed

4 files changed

+745
-150
lines changed

pkg/config/app_config.go

+35-20
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"log"
66
"os"
77
"path/filepath"
8+
"reflect"
89
"strings"
910
"time"
1011

@@ -237,7 +238,17 @@ func migrateUserConfig(path string, content []byte) ([]byte, error) {
237238

238239
// A pure function helper for testing purposes
239240
func computeMigratedConfig(path string, content []byte) ([]byte, error) {
240-
changedContent := content
241+
var err error
242+
var rootNode yaml.Node
243+
err = yaml.Unmarshal(content, &rootNode)
244+
if err != nil {
245+
return nil, fmt.Errorf("failed to parse YAML: %w", err)
246+
}
247+
var originalCopy yaml.Node
248+
err = yaml.Unmarshal(content, &originalCopy)
249+
if err != nil {
250+
return nil, fmt.Errorf("failed to parse YAML, but only the second time!?!? How did that happen: %w", err)
251+
}
241252

242253
pathsToReplace := []struct {
243254
oldPath []string
@@ -248,46 +259,52 @@ func computeMigratedConfig(path string, content []byte) ([]byte, error) {
248259
{[]string{"gui", "windowSize"}, "screenMode"},
249260
}
250261

251-
var err error
252262
for _, pathToReplace := range pathsToReplace {
253-
changedContent, err = yaml_utils.RenameYamlKey(changedContent, pathToReplace.oldPath, pathToReplace.newName)
263+
err := yaml_utils.RenameYamlKey(&rootNode, pathToReplace.oldPath, pathToReplace.newName)
254264
if err != nil {
255265
return nil, fmt.Errorf("Couldn't migrate config file at `%s` for key %s: %s", path, strings.Join(pathToReplace.oldPath, "."), err)
256266
}
257267
}
258268

259-
changedContent, err = changeNullKeybindingsToDisabled(changedContent)
269+
err = changeNullKeybindingsToDisabled(&rootNode)
260270
if err != nil {
261271
return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
262272
}
263273

264-
changedContent, err = changeElementToSequence(changedContent, []string{"git", "commitPrefix"})
274+
err = changeElementToSequence(&rootNode, []string{"git", "commitPrefix"})
265275
if err != nil {
266276
return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
267277
}
268278

269-
changedContent, err = changeCommitPrefixesMap(changedContent)
279+
err = changeCommitPrefixesMap(&rootNode)
270280
if err != nil {
271281
return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
272282
}
283+
273284
// Add more migrations here...
274285

275-
return changedContent, nil
286+
if !reflect.DeepEqual(rootNode, originalCopy) {
287+
newContent, err := yaml_utils.YamlMarshal(&rootNode)
288+
if err != nil {
289+
return nil, fmt.Errorf("Failed to remarsal!\n %w", err)
290+
}
291+
return newContent, nil
292+
} else {
293+
return content, nil
294+
}
276295
}
277296

278-
func changeNullKeybindingsToDisabled(changedContent []byte) ([]byte, error) {
279-
return yaml_utils.Walk(changedContent, func(node *yaml.Node, path string) bool {
297+
func changeNullKeybindingsToDisabled(rootNode *yaml.Node) error {
298+
return yaml_utils.Walk(rootNode, func(node *yaml.Node, path string) {
280299
if strings.HasPrefix(path, "keybinding.") && node.Kind == yaml.ScalarNode && node.Tag == "!!null" {
281300
node.Value = "<disabled>"
282301
node.Tag = "!!str"
283-
return true
284302
}
285-
return false
286303
})
287304
}
288305

289-
func changeElementToSequence(changedContent []byte, path []string) ([]byte, error) {
290-
return yaml_utils.TransformNode(changedContent, path, func(node *yaml.Node) (bool, error) {
306+
func changeElementToSequence(rootNode *yaml.Node, path []string) error {
307+
return yaml_utils.TransformNode(rootNode, path, func(node *yaml.Node) error {
291308
if node.Kind == yaml.MappingNode {
292309
nodeContentCopy := node.Content
293310
node.Kind = yaml.SequenceNode
@@ -298,15 +315,14 @@ func changeElementToSequence(changedContent []byte, path []string) ([]byte, erro
298315
Content: nodeContentCopy,
299316
}}
300317

301-
return true, nil
318+
return nil
302319
}
303-
return false, nil
320+
return nil
304321
})
305322
}
306323

307-
func changeCommitPrefixesMap(changedContent []byte) ([]byte, error) {
308-
return yaml_utils.TransformNode(changedContent, []string{"git", "commitPrefixes"}, func(prefixesNode *yaml.Node) (bool, error) {
309-
changedAnyNodes := false
324+
func changeCommitPrefixesMap(rootNode *yaml.Node) error {
325+
return yaml_utils.TransformNode(rootNode, []string{"git", "commitPrefixes"}, func(prefixesNode *yaml.Node) error {
310326
if prefixesNode.Kind == yaml.MappingNode {
311327
for _, contentNode := range prefixesNode.Content {
312328
if contentNode.Kind == yaml.MappingNode {
@@ -318,11 +334,10 @@ func changeCommitPrefixesMap(changedContent []byte) ([]byte, error) {
318334
Kind: yaml.MappingNode,
319335
Content: nodeContentCopy,
320336
}}
321-
changedAnyNodes = true
322337
}
323338
}
324339
}
325-
return changedAnyNodes, nil
340+
return nil
326341
})
327342
}
328343

0 commit comments

Comments
 (0)