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

Big optimizations for Buffer::set_rich_text #303

Merged
merged 4 commits into from
Sep 1, 2024
Merged

Conversation

UkoeHB
Copy link
Contributor

@UkoeHB UkoeHB commented Aug 31, 2024

Problem

Currently when Buffer::set_rich_text is called, all existing allocations in the buffer are simply discarded. This amounts to a lot of allocations for complex text.

Solution

Reuse existing allocations as much as possible when rebuilding a buffer.

Note that the changes here try to line up buffer reuse so it all maps back into the same positions in the buffer if possible. This way if, for example, you are just animating text color, the buffer won't actually require any extra allocations after the first time it's constructed (and won't over-allocate).

I did not originally intend for this PR to get so big, but diving into the code was a rabbit hole of allocations to squash.

Closes #301

Testing

I did not do any perf testing.

Follow-up

Additional areas for improvement:

  • Buffer::set_text can also be optimized in a similar fashion.
  • Use SmallVec instead of Vec where it makes sense. This should somewhat reduce the cost to make new buffers.
  • There is one allocation at the top of set_rich_text that I didn't try to solve yet.
  • Internally, ShapeLine is essentially a Vec<Vec<Vec<ShapeGlyph>>>. This can be flattened into two vecs: a vec of indices + metadata and a vec of ShapeGlyphs. Or even one vec with the metadata in-line.
  • Internally, BufferLine caches a Vec<LayoutLine>, which is essentially Vec<Vec<LayoutGlyph>>. This could also be flattened.
  • Buffer caches a ShapeBuffer but it would be better to have this in FontSystem or elsewhere for global reuse.

jackpot51
jackpot51 previously approved these changes Sep 1, 2024
@jackpot51
Copy link
Member

The Rust/build CI will need to pass

@UkoeHB
Copy link
Contributor Author

UkoeHB commented Sep 1, 2024

@jackpot51 fixed. Looks like cargo-deny needs to be updated on your end.

@jackpot51 jackpot51 merged commit c65f299 into pop-os:main Sep 1, 2024
1 of 2 checks passed
@UkoeHB UkoeHB deleted the richtext branch September 1, 2024 21:17
@UkoeHB UkoeHB mentioned this pull request Sep 2, 2024
13 tasks
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.

Efficiency of Buffer::set_rich_text
2 participants