-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix preloader mode in benchmarks #6359
Conversation
Can you please fix vet? I'll take a look at this next week :). |
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.
Some minor nits. Looks solid overall though. I like everything going through helpers.
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.
Seems mostly OK. Some minor comments.
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) |
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.
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.
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.
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.
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.
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.
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. Thanks for the contribution.
I don't have the rights to merge it, can somebody please do it for me? |
Thank you for your contribution! |
This PR fixes #6320
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 likemarshal
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