[pull] master from jesseduffield:master #343
Merged
+1,039
−186
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![pull[bot]](https://avatars.githubusercontent.com/in/12910?s=60&v=4){kind=small}
- **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
This was backwards; we renamed Sha to Hash, so Sha is now deprecated, not Hash.
It has been 'e' instead of 'o' for quite a while now.
It is false by default. This way there's one less change to make in the next commit.
This allows us to tell whether they appear in the user's config file, which we will need later in this branch.
We'll reuse it in the next commit.
- **PR Description** I want to be able to configure custom commands that I don't need very often; I don't want these to pollute the global keybindings menu, and I don't want to assign globally unique keybindings to them (because there are only so many of these available, and also because I wouldn't be able to remember them, because the commands are not used often). However, I still want to invoke them through keybindings somehow. I find that the perfect solution for this is to configure a menu that contains custom commands. I can pop open the menu using only one key that I need to remember, but I can access the individual custom commands inside using keys that don't need to be unique with the rest of the global keybindings. In this PR we achieve this by adding an optional `subCommands` property to customCommand that can be used instead of the other properties like `command`, etc. This is an alternative approach to #4276, which added a new top-level property for custom command menus. Potentially closes #3799. - **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)) * [x] Tests have been added/updated (see [here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md) for the integration test guide) * [x] Text is internationalised (see [here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation)) * [x] 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)) * [x] Docs have been updated if necessary * [x] You've read through your own file changes for silly mistakes etc
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.1)
Can you help keep this open source service alive? 💖 Please sponsor : )