Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

status: optimize GRPCStatus() calls #6539

Merged
merged 1 commit into from
Aug 15, 2023
Merged

status: optimize GRPCStatus() calls #6539

merged 1 commit into from
Aug 15, 2023

Conversation

ash2k
Copy link
Contributor

@ash2k ash2k commented Aug 11, 2023

RELEASE NOTES: none

@arvindbr8
Copy link
Member

Hi @ash2k, seems like the GRPCStatus() is only a getter and side effects (if any) arent obvious to me now. I'm curious how did you find this? Has this caused some sort of regression in callsites?

@ash2k
Copy link
Contributor Author

ash2k commented Aug 11, 2023

Hi. It's a getter on gRPC's implementations, but it might be doing something in other implementations. It shouldn't have any side effects, but it may be allocating extra memory (e.g. to construct the object its returning), which is not required to do twice.

It didn't cause any problems, I was just looking at the gRPC code while working on my project and spotted this.

Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diff lgtm.

I'm less concerned about the double calls since gs.GRPCStatus() is only a getter and does not have other side-effects (for eg, incrementing a counter).

cc: @easwars for another look

@arvindbr8 arvindbr8 requested a review from easwars August 14, 2023 16:12
@arvindbr8 arvindbr8 changed the title Call GRPCStatus() only once status: optimize GRPCStatus() calls Aug 14, 2023
@arvindbr8 arvindbr8 added the Type: Internal Cleanup Refactors, etc label Aug 14, 2023
@arvindbr8 arvindbr8 added this to the 1.58 Release milestone Aug 14, 2023
@arvindbr8
Copy link
Member

closing and opening to re-run new tests

@arvindbr8 arvindbr8 closed this Aug 14, 2023
@arvindbr8 arvindbr8 reopened this Aug 14, 2023
@ash2k
Copy link
Contributor Author

ash2k commented Aug 15, 2023

I don't know why the vet-proto job is failing. I've just rebased the branch but it didn't help.

@easwars
Copy link
Contributor

easwars commented Aug 15, 2023

I don't know why the vet-proto job is failing. I've just rebased the branch but it didn't help.

vet-proto does not block merge. I've sent #6551 to fix the failure in the meantime.

@easwars easwars merged commit 3e92504 into grpc:master Aug 15, 2023
1 check passed
@ash2k ash2k deleted the call-once branch August 15, 2023 23:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 12, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants