core: add Metadata.discardAll() #2236
Conversation
|
@ejona86 FYI |
|
LGTM. Generally, I am unsure whether it's a good idea to change public APIs for small performance improvements. Even if those methods/interfaces are marked experimental or internal. Well, I think designing APIs with performance in mind is a good idea. Say if we can show an impact of 1%, 2% or more. For example, taking the recents changes in #2238 , #2233 and this PR together. How much does that buy us in terms of real QPS / latency? Do you know? |
| * optimization if you do not need the previous values. | ||
| */ | ||
| @ExperimentalApi | ||
| public <T> void discardAll(Key<T> key) { |
|
@carl-mastrangelo LGTM. Hopefully the failure to update |
Metadata.removeAll creates an iterator for looking through removed values even if the call doens't use it. This change adds a similar method which doesn't create garbage. This change makes it easier in the future to alter the internals of Metadata where it may be expensive to return removed values.
carl-mastrangelo
force-pushed
the
parseMeMaybe
branch
from
08f688e to
f78644d
Compare
|
@ejona86 yep, that was it. @buchgr I know sold this as a perf CL, but really I think the API is better this way. Every single call of removeAll in our own code base didn't use the results. The If we weren't maintaining API compat, I would just change removeAll to not return, and add a special method for returning those values. |
3 participants
Metadata.removeAll creates an iterator for looking through removed
values even if the call doens't use it. This change adds a similar
method which doesn't create garbage.
This change makes it easier in the future to alter the internals
of Metadata where it may be expensive to return removed values.
Found by using Yourkit. I personally would have expected the JVM to remove those as dead code, but apparently it does not.