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.

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

Textarea is slow when pasting things from the clipboard #831

Closed
cruizba opened this issue Oct 4, 2023 · 19 comments · Fixed by charmbracelet/bubbles#427
Closed

Textarea is slow when pasting things from the clipboard #831

cruizba opened this issue Oct 4, 2023 · 19 comments · Fixed by charmbracelet/bubbles#427
Labels

Comments

@cruizba
Copy link

cruizba commented Oct 4, 2023

Describe the bug
Copy and paste things in textarea breaks somehow the fast user experience while using it in a CLI tool.
When you paste a large text into a textarea, all characters are written one by one.
Also, I've noticed that, the larger the text is and the more number of lines it has, user input slows down incrementally.

Setup
Please complete the following information along with version numbers, if applicable.

  • OS: Ubuntu
  • Shell: zsh
  • Terminal Emulator: Terminator

To Reproduce
Steps to reproduce the behavior:

  1. Execute the text area tutorial, change the CharLimit of the TextArea to a high value (so large texts can be pasted) and paste a large file. I am actually pasting a PEM file (a certificate) which has 28 lines and 65 characters per line.
  2. See how the text is being copied character by character, and it slowed down the more characters are currently in the textarea

Source Code
https://github.com/charmbracelet/bubbletea/tree/master/examples/textarea

Expected behavior
It should be fast on pasting things to not break the user experience.

@cruizba cruizba changed the title Textarea slow when pasting things from the clipboard Textarea is slow when pasting things from the clipboard Oct 4, 2023
@meowgorithm
Copy link
Member

Hi! Thanks for the report. I believe there two items that will improve the situation here:

@cruizba
Copy link
Author

cruizba commented Oct 4, 2023

Oh, I did not see there were current issues and PRs about this 🤔

Amazing project BTW 🦾

@wesen
Copy link

wesen commented Oct 16, 2023

I have "hacked" textarea to be significantly faster. After profiling, I found wrap() to be the biggest culprit, as it is called by LineInfo which in turn is called by the CursorPosition method.

image

Here is a sketch of the solution, I built a LRU memoization cache here: https://github.com/go-go-golems/clay/blob/main/pkg/memoization/memoization.go

Here is the "hacked" textarea: wesen/bubbles@30a4341

I was able to easily copy paste longer sections of text: https://asciinema.org/a/Dab4MlcIxaWinhJwQC3kU9fCV

@cruizba
Copy link
Author

cruizba commented Oct 17, 2023

@wesen That's actually a clever solution without changing anything of the rest of the bubbletea source code.

Why don't you do a PR and include the memoization.go file in the PR (maybe they don't want to add more dependencies into the project)?

I am not a mantainer of the library or anything, just a regular user, but from my point of view this does not looks hard to integrate. It could be a temporal solution before they integrate this PR (#397)

@meowgorithm what do you think?

@decentral1se
Copy link

decentral1se commented Dec 4, 2023

Yeh, just chiming to say that textarea copy/pasta is not really that usable at all atm due to this performance issue. Thanks for raising the issue!

@cruizba
Copy link
Author

cruizba commented Dec 11, 2023

Bumping

@nadimkobeissi
Copy link

Please merge #427!

@meowgorithm
Copy link
Member

meowgorithm commented Jan 9, 2024

Hey everyone. Just a note that we're looking into bracketed paste (#397) which should alleviate some of the performance woes. It's a delicate one to execute on properly, and will need some additional changes beyond that PR in practice, so note we're examining the solution carefully.

@nadimkobeissi #427 was merged a long time ago. Perhaps a typo?

@mikelorant
Copy link

Have been doing profiling of this issue and discovered a very simple change that causes a nearly doubling of performance. Swapping out the StringWidth function from go-runewidth to the same function in uniseg. Since go-runewidth uses uniseg under the hood, this does not have a change on the imports (just requires a bump to uniseg 0.4.4.

Benchmark results

Function Count Speed
BenchmarkStringWidthRunewidth-4 3397 325152 ns/op
BenchmarkStringWidthUniseg-4 12072 101341 ns/op

Code used for benchmarks:

func BenchmarkStringWidthMatt(b *testing.B) {
    loremIpsumGenerator := loremipsum.NewWithSeed(1234)
    str := loremIpsumGenerator.Paragraph()

    for i := 0; i < b.N; i++ {
        runewidth.StringWidth(str)
    }
}

func BenchmarkStringWidthUniseg(b *testing.B) {
    loremIpsumGenerator := loremipsum.NewWithSeed(1234)
    str := loremIpsumGenerator.Paragraph()

    for i := 0; i < b.N; i++ {
        uniseg.StringWidth(str)
    }
}

Iteration count was only 1 so this was testing StringWidth only.

I would also recommend replacing the RuneWidth function, however, uniseg has not made this public. I have requested for this function to be changed from private to public so it could also be used. This would then allow go-runewidth to be dropped entirely.

Be aware, for the full benefits, I would recommend this change being applied to lipgloss and reflow as well.

I have no problems putting up the pull requests for all impacted repositories, however I would like confirmation this aligns with the project directions before I begin the work.

@meowgorithm
Copy link
Member

Well that's awesome. We've noticed rivo/uniseg added a StringWidth to the API but weren't aware the benefits are so great. We'll submit PRs accordingly for Bubbles, Lip Gloss, reflow and termenv today.

@maaslalani
Copy link
Contributor

We just merged memoization to improve textarea performance: charmbracelet/bubbles#427

Thank you for the well-written issue @cruizba and for the well-done implementation @wesen!

@maaslalani
Copy link
Contributor

maaslalani commented Jan 22, 2024

Well that's awesome. We've noticed rivo/uniseg added a StringWidth to the API but weren't aware the benefits are so great. We'll submit PRs accordingly for Bubbles, Lip Gloss, reflow and termenv today.

@mikelorant, thanks so much for the information and benchmarks. We've swapped in uniseg for Bubbles (textinput and textarea) and have opened PRs for termenv and reflow!

When the RuneWidth function becomes available to use, we will swap those out too, if it performance is improved!

@mikelorant
Copy link

Unfortunately, RuneWidth adds significantly to this complexity. @rivo has provided some detailed feedback on why using this function causes problems.

That's because a single code point (rune) is not always a complete character. That's the whole basis of the uniseg package. The README explains this in detail. One example given there is the country flag emoji. These emojis must always consist of two runes. What's the width of only one of these runes then? It's basically undefined because it's an incomplete grapheme cluster. The mattn/go-runewidth package gets this fundamentally wrong and I have tried for a long time to help them do it right but there was never much interest in following up on it.
rivo/uniseg#48 (comment)

However, for the sake of complete benchmarks I have compared the two packages.

Function Count Speed
BenchmarkRuneWidthRunewidth-4 335290 3287 ns/op
BenchmarkRuneWidthUniseg-4 24157 49827 ns/op

Code used for benchmarks:

func BenchmarkRuneWidthRunewidth(b *testing.B) {
    loremIpsumGenerator := loremipsum.NewWithSeed(1234)
    str := loremIpsumGenerator.Paragraph()

    for i := 0; i < b.N; i++ {
        for _, r := range str {
            runewidth.RuneWidth(r)
        }
    }
}

func BenchmarkRuneWidthUniseg(b *testing.B) {
    loremIpsumGenerator := loremipsum.NewWithSeed(1234)
    str := loremIpsumGenerator.Paragraph()

    for i := 0; i < b.N; i++ {
        for _, r := range str {
            uniseg.StringWidth(string(r))
        }
    }
}

The easy answer, there is no benefit to swap out the RuneWidth function.

The complicated answer, we should never be using go-runewidth RuneWidth anywhere as it is a fundamentally broken approach.

I do not wish to add to this closed issue with further details on how we should approach the replacement of RuneWidth so will open a new issue for discussion.

@rivo
Copy link

rivo commented Jan 27, 2024

@mikelorant I've spent some time today working on uniseg's performance. You may want to check out the current version (v0.4.6). While it may still not be as fast as go-runewidth's RuneWidth function (which is expected, as it doesn't handle grapheme clusters), you should see large improvements.

For the reasons I mentioned in the comment you quoted above, I would advise against handling runes in isolation, as you do in the BenchmarkRuneWidthUniseg function. You'll want to do it like in this example. Every iteration of the loop will give you a full "character" (i.e. grapheme cluster), along with its width.

@mikelorant
Copy link

@rivo Can confirm some very nice performance benefits.

I've also taken the opportunity to continue to do some CPU profiling and think you may have 2 places worth investigating if you want to squeeze some more improvements in.

CleanShot 2024-01-29 at 09 41 37

I may be very wrong about this (fairly new to CPU profiling) so I may not be reading this correctly.

@rivo
Copy link

rivo commented Jan 29, 2024

The property function is already a binary search, while fast-tracking ASCII characters. I don't see how this can be improved.

It seems to me that you're still using the Graphemes class. As mentioned in my other comments (and now also in the package comments), this class implements all functionality of the package. Do you really need sentence boundaries, word boundaries, and line break determination? If you're really just interested in grapheme clusters and widths, you'll want to switch to FirstGraphemeClusterInString. It'll save you a lot more CPU cycles.

@mikelorant
Copy link

Will reply to you on the reflow issue as it will have more context with the other benchmarks. Good news though, performance is better than it originally was!

@mikelorant
Copy link

Absolutely giant increase in performance with muesli/reflow#71. All thanks to @rivo for this one.

@wesen You might want to try updating your reflow to the branch I pushed. I think combining both your pull request and mine we may have dealt with most of the issues. I still think more improvements are possible but this seems like a pretty big jump compared to how it performed before.

@mikelorant
Copy link

Just did some very quick benchmarks of my own app with pasting 1000 characters (14 lines) into a textarea of 72 characters wide:

Before: 42s
After: 3s

For context, no optimisations to my own app code at all. All these performance increases came from:

  • bubbles
  • lipgloss
  • reflow
  • uniseg

Very happy with the results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants