Pass shared PietText by ref instead of cloning #1205
Merged
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
cmyr
force-pushed
the
pass-text-by-ref
branch
from
75fe81c
to
a5fefc7
Compare
raphlinus
approved these changes
Sep 9, 2020
There was a problem hiding this comment.
Yes, looks good. When you say a few microseconds, did you measure that?
In theory, switching to &mut might break something, but in practice I think it's a cleaner type. It's causing flashbacks to the early days of xi-win, where it was a bit of a struggle to get the lifetimes right when rebuilding resources (tons of cloning of these factories), but I think generally the current state is right.
|
no, microseconds was just me mentally multiplying the cost of cloning an arc by a few hundred text objects. |
|
Yeah, the new changes reflect breakage I would have expected, but it doesn't seem bad at all to fix them. +1 |
Currently we create a copy of the PietText object every time we need one. This changes that so that there is a single instance in the ContextState, and the various contexts just pass out a reference to this object as needed. This should have few practical implications, except that in trees with lots of text we will save a few microseconds. It's mostly an API improvement, since the methods that tend to use a PietText object generally want it passed by ref anyway.
cmyr
force-pushed
the
pass-text-by-ref
branch
from
a5fefc7
to
7046fa9
Compare
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.
Currently we create a copy of the PietText object every time we
need one. This changes that so that there is a single instance
in the ContextState, and the various contexts just pass out
a reference to this object as needed.
This should have few practical implications, except that in
trees with lots of text we will save a few microseconds. It's
mostly an API improvement, since the methods that tend to use
a PietText object generally want it passed by ref anyway.