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.

Sign up for GitHub

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

Jump to bottom

key: optimize Matches #261

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

key: optimize Matches #261

wants to merge 2 commits into from

Conversation

knz
Copy link
Contributor

@knz knz commented Oct 2, 2022 •
edited

Fixes #262.

name                old time/op    new time/op    delta
Matches/success-32    66.9ns ± 1%    15.7ns ± 0%   -76.47%  (p=0.016 n=5+4)
Matches/fail-32       27.9ns ± 0%    12.4ns ± 2%   -55.70%  (p=0.016 n=4+5)

name                old alloc/op   new alloc/op   delta
Matches/success-32     5.00B ± 0%     0.00B       -100.00%  (p=0.008 n=5+5)
Matches/fail-32        0.00B          0.00B           ~     (all equal)

name                old allocs/op  new allocs/op  delta
Matches/success-32      1.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)
Matches/fail-32         0.00           0.00           ~     (all equal)

cc @meowgorithm @muesli

@knz
feat: benchmark the key match operation
d497a37
@knz
fix: optimize the key binding match operation
43a92fb 
This reduces the complexity of the comparison and gets rid of the heap
allocation on every match.

Benchmark before/after (`go test -bench . -benchtime 3s -count 5 -benchmem`):
```
name                old time/op    new time/op    delta
Matches/success-32    66.9ns ± 1%    15.7ns ± 0%   -76.47%  (p=0.016 n=5+4)
Matches/fail-32       27.9ns ± 0%    12.4ns ± 2%   -55.70%  (p=0.016 n=4+5)

name                old alloc/op   new alloc/op   delta
Matches/success-32     5.00B ± 0%     0.00B       -100.00%  (p=0.008 n=5+5)
Matches/fail-32        0.00B          0.00B           ~     (all equal)

name                old allocs/op  new allocs/op  delta
Matches/success-32      1.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)
Matches/fail-32         0.00           0.00           ~     (all equal)
```
@knz
Copy link
Contributor Author

knz commented Oct 2, 2022 •
edited

NB: I could imagine the map from string to tea.Key being defined in the bubbletea package directly. If you prefer that, let me know.

@muesli
Copy link
Member

muesli commented Oct 5, 2022

NB: I could imagine the map from string to tea.Key being defined in the bubbletea package directly. If you prefer that, let me know.

I think that makes sense and is probably nicer next to the other tea.Key code.

@maaslalani
Copy link
Member

Whoa! This is awesome @knz, solid optimization!

@knz knz mentioned this pull request Oct 6, 2022
Open
@knz
Copy link
Contributor Author

knz commented Oct 6, 2022

I think that makes sense and is probably nicer next to the other tea.Key code.

Ok see charmbracelet/bubbletea#491

Then to merge this PR I'll need a release number for the go.mod update.

This was referenced Oct 7, 2022
Merged
Open
@maaslalani
Copy link
Member

@muesli Is this good to merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

key: string-based parsing is inefficient
3 participants
@knz @muesli @maaslalani