-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
core, api, benchmarks: Random acts of garbage reduction #7375
Conversation
I noticed some opportunities to reduce allocations on hot paths
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.
The UnsafeByteOperations change is the only reason I'm not approving right now (although I wouldn't merge). I do think there are some tweaks that could be made.
core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java
Outdated
Show resolved
Hide resolved
return offset + length; | ||
} | ||
}; | ||
|
||
@Override | ||
public void readBytes(final byte[] dest, final int destOffset, int length) { |
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.
You might want to clean up the now-unnecessary final
s. Not a big deal.
Thanks @ejona86! I will address the comments when I get a chance but likely won't be until next week. |
@njhill, that's more than fine. I'm sorry it took so very long for us to review. And in case it wasn't clear, I'm fine with accepting the garbage reduction changes that probably don't reduce garbage. It's a PITA to check whether the JIT is actually doing what we hope and that still depends on what side of the bed the JIT woke up on. Since the changes don't harm readability, they seem fine. But I pointed them out since the value of those sorts of changes is questionable and we may not want to get out-of-hand with that class of change. |
@ejona86 I think I've addressed all the comments, PTAL I did also try to rebase this branch but it wouldn't let me push that:
|
@njhill, that error doesn't look related to the force push. We have a github workflow now, so it looks like that error would apply every time you try to push to your repo with newer PRs. I don't know how you are authenticating, but maybe you need to go through that process again so it can request more more scopes. |
@voidzcy, could you review? |
CI failures need to be addressed (missing imports). |
Thank you @njhill! |
I noticed some opportunities to reduce allocations on hot paths