-
Notifications
You must be signed in to change notification settings - Fork 4k
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
[Skylark] Avoid unnecessary copyOf invocations. #5699
Conversation
67670c6
to
cc76496
Compare
I'm a bit surprised by this change. I thought |
@laurentlb, I thought so too, but according to source that's not the case :( |
just to be clear - there are no allocations involved in |
@@ -319,7 +319,7 @@ public static boolean toBoolean(Object o) { | |||
for (Map.Entry<?, ?> entries : dict.entrySet()) { | |||
list.add(entries.getKey()); | |||
} | |||
return ImmutableList.copyOf(list); | |||
return ImmutableList.copyOf(list); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random driveby while skimming the PR & seeing the whitespace bump on this line: Convert this over to an ImmutableList.builderWithExpectedSize()
also?
It presumably isn't getting called much if your numbers didn't complain about it too, but it's making an ArrayList
only to have ImmutableList
immediately copy it all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, @deadmoose! I'm gradually going through all of them, but before I'm trying to use either JMH or async-profiler to see the impact, so it's a bit slow :) I'll shoot a PR with this change as soon as I confirm in JMH that it makes things faster. Most likely your suggestion should improve things, since my previous testing showed that ImmutableList.builder
+ build
is interestingly faster than ArrayList.add
, but I'll double check.
Looks good! Can you add a comment to the function to explain why it was added? |
Thanks for taking a look, @laurentlb! I'll add a comment. |
Java uses dynamically generated proxy classes to access annotation properties and their methods are ~7X slower than plain getters. According to async-profiler 50%+ of `convertArgumentList` method time is spent in dynamic proxy methods, so optimizing their performance makes sense. This also makes the model less anemic, since POJOs can actually provide business methods.
This change is focused on 2 things: - avoid creating builders in case they don't end up being used - create builders using the maximum expected size to avoid intermediate allocations to accomodate more elements
Java uses dynamically generated proxy classes to access annotation properties and their methods are ~7X slower than plain getters. According to async-profiler 50%+ of `convertArgumentList` method time is spent in dynamic proxy methods, so optimizing their performance makes sense. This also makes the model less anemic, since POJOs can actually provide business methods.
According to async-profiler, about 50% of `Tuple.getSlice` method invocation is spent in `ImmutableList.copyOf` which is completely unnecessary for cases when an `ImmutableList` instance is passed.
rebased, resolved conflicts and added a comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
According to async-profiler, about 50% of
Tuple.getSlice
method invocation is spent inImmutableList.copyOf
which is completely unnecessary for cases when anImmutableList
instance is passed.