Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix preloader mode in benchmarks #6359

Merged
merged 5 commits into from
Jul 11, 2023
Merged

Fix preloader mode in benchmarks #6359

merged 5 commits into from
Jul 11, 2023

Conversation

s-matyukevich
Copy link
Contributor

@s-matyukevich s-matyukevich commented Jun 7, 2023 •
edited by zasweq
Loading

This PR fixes #6320

  • It makes sure the benchmarks use preloaded message not only on the client but on the server as well if preloader=on option is provided.
  • It adds support for preloader=on option in streaming mode (previously it was supported only in unconstrained mode) Initially I was going to add it to unary mode as well, but this would require using raw clientConnection instead of generated grpc stubs, so I decided to ask before doing that.

This PR ends up being not super useful for the use-case I had in mind for it. The problem I was trying to solve is that when testing #6309 I discovered that google.golang.org/protobuf/proto.MarshalOptions.marshal takes the wast majority of the inuse memory in the resulting memroy profile. I thought that by fixing preloader I'll be able to reduce the impact of this method on the overall memory usage and therefore make the impact of memory optimizations more visible. What ends up happening is that profiler results are misleading. Looks like marshal doesn't have significant impact on the overall memory usage - it is just a method that take relatively long time to execute, so memory allocated by it sometimes (but not always) ends up being counted in the inuse_memory profile, but all this memory is immediately deallocated after the method finishes. So enabling preloader does significantly reduce allocations per operation (which is expected because we are not calling marshal for every operation) but it doesn't have significant impact on the amount of memory reserved by the process in the operating system.

Still, I decided to submit this PR as you might find it useful for testing other use-cases, like this one #2432

RELEASE NOTES: N/A

@zasweq zasweq self-requested a review June 8, 2023 04:01
@zasweq zasweq self-assigned this Jun 10, 2023
@zasweq zasweq added Type: Feature New features or improvements in behavior Type: Bug and removed Type: Feature New features or improvements in behavior labels Jun 10, 2023
@zasweq zasweq added this to the 1.57 Release milestone Jun 10, 2023
@zasweq
Copy link
Contributor

zasweq commented Jun 10, 2023

Can you please fix vet? I'll take a look at this next week :).

@zasweq zasweq requested review from arvindbr8 and removed request for zasweq June 14, 2023 17:35
@zasweq zasweq assigned arvindbr8 and zasweq and unassigned zasweq and arvindbr8 Jun 14, 2023
@zasweq zasweq requested review from zasweq and removed request for arvindbr8 June 14, 2023 22:52
zasweq
zasweq requested changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

Some minor nits. Looks solid overall though. I like everything going through helpers.

benchmark/benchmain/main.go Outdated Show resolved Hide resolved
benchmark/benchmark.go Outdated Show resolved Hide resolved
benchmark/benchmain/main.go Outdated Show resolved Hide resolved
@zasweq zasweq assigned easwars and unassigned zasweq Jun 22, 2023
@zasweq zasweq requested a review from easwars June 22, 2023 19:31
easwars
easwars requested changes Jun 27, 2023
View reviewed changes
Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

Seems mostly OK. Some minor comments.

benchmark/benchmain/main.go Outdated Show resolved Hide resolved
benchmark/benchmain/main.go Outdated Show resolved Hide resolved
benchmark/benchmain/main.go Outdated Show resolved Hide resolved
benchmark/benchmain/main.go
preparedMsg[cn][pos] = &grpc.PreparedMsg{}
err := preparedMsg[cn][pos].Encode(stream, req)
if err != nil {
logger.Fatalf("%v.Encode(%v, %v) = %v", preparedMsg[cn][pos], req, stream, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to return an error from here instead of calling Fatal. Please see:
https://google.github.io/styleguide/go/best-practices#program-checks-and-panics and https://google.github.io/styleguide/go/decisions#dont-panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be a common pattern used in benchmarks logger.Fatal is used 6 times in the same file. If we want to propagate the error up to the main method and handle it there we should fix all the cases, I can do it in the same PR, but this would be an unrelated change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok fair enough. If you have cycles, and would like to fix this in a follow-up PR, I would be happy to review it. Thanks.

benchmark/benchmark.go Outdated Show resolved Hide resolved
benchmark/benchmark.go Outdated Show resolved Hide resolved
@easwars easwars assigned s-matyukevich and unassigned easwars Jun 27, 2023
@s-matyukevich
Copy link
Contributor Author

@easwars @zasweq - I fixed all comments except for the one that is left unresolved. Let me know what I should do about it.

zasweq
zasweq approved these changes Jun 27, 2023
View reviewed changes
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution.

@s-matyukevich
Copy link
Contributor Author

I don't have the rights to merge it, can somebody please do it for me?

@easwars easwars merged commit db32c5b into grpc:master Jul 11, 2023
1 check passed
@easwars
Copy link
Contributor

easwars commented Jul 11, 2023

Thank you for your contribution!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zasweq zasweq zasweq approved these changes

@easwars easwars easwars approved these changes

Assignees

@s-matyukevich s-matyukevich

@zasweq zasweq

Labels
Type: Bug
Projects
None yet
Milestone
1.57 Release
Development

Successfully merging this pull request may close these issues.

preloader=on flag in benchmarks impacts only the client and not the server
4 participants
@s-matyukevich @zasweq @easwars @arvindbr8