Skip to content

Optimizations for TextView#268

Closed
urandom wants to merge 2 commits intorivo:masterfrom
urandom:textview-optimizations
Closed

Optimizations for TextView#268
urandom wants to merge 2 commits intorivo:masterfrom
urandom:textview-optimizations

Conversation

@urandom
Copy link
Copy Markdown

@urandom urandom commented Apr 14, 2019

Only clear the index when the textview dimensions change or the text
itself is cleared.
Add to the index when new text is added to the view.

Fixes #266

urandom added 2 commits April 14, 2019 17:29
Only clear the index when the textview dimensions change or the text
itself is cleared.
Add to the index when new text is added to the view.

Fixes rivo#266
Add a parameter to indicate whether the caller wants the string width.
Do not calculate the string width when it is not needed
tslocum added a commit to tslocum/tview that referenced this pull request Oct 18, 2019
tslocum added a commit to tslocum/tview that referenced this pull request Oct 19, 2019
@tslocum
Copy link
Copy Markdown
Contributor

tslocum commented Oct 20, 2019

I've created benchmarks for writing to and drawing TextViews (submitted as #356) and compared the results before and after applying these optimizations.


go test -run=NONE -bench=. -count=3

Before

After


benchcmp -best -changed

Comparison

@Bios-Marcel
Copy link
Copy Markdown
Contributor

So, it has gotten worse? Because after shows more ns/operation and such

@tslocum
Copy link
Copy Markdown
Contributor

tslocum commented Oct 21, 2019

Based on the benchmark results, these optimizations:

  • Increase the amount of time it takes to write to unwrapped TextViews by 350%
  • Do not affect the amount of time it takes to write to wrapped TextViews
  • Increase the number of allocations when writing to both unwrapped and wrapped TextViews by 1000%
  • Increase the allocation size when writing to unwrapped TextViews by 500%
  • Do not affect the allocation size of writing to wrapped TextViews
  • Reduce the amount of time it takes to draw unwrapped TextViews by 65%
  • Reduce the amount of time it takes to draw wrapped TextViews by 20%
  • Reduce the allocation size when drawing unwrapped TextViews by 40%
  • Reduce the allocation size when drawing wrapped TextViews by 20%

@urandom
Copy link
Copy Markdown
Author

urandom commented Oct 21, 2019

This change optimizes for large amounts of text, especially one that constantly gets appended to (displaying a tailing log).
Your benchmarks present a static text of 512 bytes, where performance is quite insignificant.

@tslocum
Copy link
Copy Markdown
Contributor

tslocum commented Oct 23, 2019

Thanks for your feedback. I've updated the tests and benchmarks to include cases where a small amount of data is written to a TextView that contains a lot of data. You can see these changes in the updated pull request I linked earlier.

Updated results and comparison

@foxcpp
Copy link
Copy Markdown

foxcpp commented Apr 24, 2020

Any chance to have this merged? I am currently using tview for TUI for my experimental chat application and use TextView as a chat log.

@foxcpp
Copy link
Copy Markdown

foxcpp commented Apr 24, 2020

@tslocum, hey, since you are maintaining cview, what do you think about getting that PR in? Imo this is a good improvement. I would submit it but there are some non-trivial conflicts so I am not sure about them.

@foxcpp
Copy link
Copy Markdown

foxcpp commented Apr 24, 2020

I rebased it here: https://github.com/foxcpp/tview/tree/textview-opts but as I said, I am not sure if I did everything correctly.
Also included my small optimization from #435.

@tslocum
Copy link
Copy Markdown
Contributor

tslocum commented Apr 24, 2020

Thanks @foxcpp. I am interested in merging your changes into cview. I'll let you know once I've had a chance to review your changes.

@tslocum
Copy link
Copy Markdown
Contributor

tslocum commented Apr 26, 2020

Hey @foxcpp, I only see your optimization to use TrimRightFunc in your updated branches. Were you able to review the benchmark results which seem to show these changes cause decreases in performance?

@foxcpp
Copy link
Copy Markdown

foxcpp commented Apr 27, 2020

@tslocum Woops, forgot to git push. Now it should be there. Also added your benchmarks.

Here is the benchcmp output master vs textview-opts branch: benchcmp.txt

There is a decrease in performance for Draw almost everywhere, enough to nullify my TrimRightFunc change, huh. Write gets +500% improvement for scrollable TextView, though.

@tslocum
Copy link
Copy Markdown
Contributor

tslocum commented Apr 27, 2020

I believe you have the results backwards: when not wrapping text, Draw is improved 60% while Write is up to 600% slower (if old is master and new is textview-opts).

@foxcpp
Copy link
Copy Markdown

foxcpp commented Apr 27, 2020

Woops x2. You are right.

@rivo
Copy link
Copy Markdown
Owner

rivo commented Aug 26, 2023

Closing this as it's obsolete. Check out the new TextView implementation for details.

@rivo rivo closed this Aug 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TextView consumes too much CPU during reindexing/drawing

5 participants