core, api, benchmarks: Random acts of garbage reduction#7375
core, api, benchmarks: Random acts of garbage reduction#7375ejona86 merged 3 commits intogrpc:masterfrom
Conversation
I noticed some opportunities to reduce allocations on hot paths
ejona86
left a comment
There was a problem hiding this comment.
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
| }; | ||
|
|
||
| @Override | ||
| public void readBytes(final byte[] dest, final int destOffset, int length) { |
There was a problem hiding this comment.
You might want to clean up the now-unnecessary finals. 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