Don't copy Glyph on GlyphCache hit #6954
Draft
+90
−47
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
The Glyph is being passed around by reference, so doing the copy is not desired.
kchibisov
commented
May 25, 2023
| @@ -192,72 +200,92 @@ impl GlyphCache { | |||
| &mut self, | |||
| glyph_key: GlyphKey, | |||
| loader: &mut L, | |||
| show_missing: bool, | |||
| ) -> Glyph | |||
| ) -> Result<&Glyph, Box<RasterizerError>> | |||
There was a problem hiding this comment.
The error happens once per glyph key, so to not enlarge the return value we simply box the value.
chrisduerr
requested changes
May 25, 2023
|
The glyph is ~32 bytes and we copy it for each cell, so if we assume 77 * 180 cells we copy 400 KB of data for no reason for each frame, given that we only use a reference, and not the copy. And with my code we copy 4x less, so it's simply faster? |
|
I'm pretty sure our |
Says who? Unless you've benchmarked it, it's not faster. Making the code more complicated if there's no real world benefit to it isn't useful. |
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.
The Glyph is being passed around by reference, so doing the copy is not desired.
--
It might look ugly a bit given that missing glyph handling must be moved, but it's actually
don't try to copy lots of glyph data around on each renderer call.