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

profiling: add hooks within grpc #3159

Merged
merged 5 commits into from
Feb 12, 2020
Merged

profiling: add hooks within grpc #3159

merged 5 commits into from
Feb 12, 2020

Conversation

adtac
Copy link
Contributor

@adtac adtac commented Nov 7, 2019

No description provided.

@adtac adtac mentioned this pull request Nov 7, 2019
Closed
@dfawley dfawley added the Type: Performance Performance improvements (CPU, network, memory, etc) label Nov 11, 2019
@dfawley dfawley added this to the 1.26 Release milestone Nov 11, 2019
@adtac adtac mentioned this pull request Nov 14, 2019
Merged
@dfawley
Copy link
Member

dfawley commented Dec 11, 2019

Is this PR ready for review once you merge in the changes from #3158?

@adtac
Copy link
Contributor Author

adtac commented Dec 11, 2019

Yes. Has been ready for review from day one btw :P

@dfawley
Copy link
Member

dfawley commented Dec 12, 2019

Yes. Has been ready for review from day one btw :P

As a general rule, I prefer if travis is green before reviews start, just to be sure it doesn't need potentially significant changes in order to make tests pass. Or if changes in prerequisite PRs are needed, that significant changes aren't needed here either. I assume that's not the case here, but were you planning to rebase this PR soon? Thanks!

@dfawley dfawley modified the milestones: 1.26 Release, 1.27 Release Dec 17, 2019
@adtac
Copy link
Contributor Author

adtac commented Dec 18, 2019

@dfawley rebased onto latest master

@adtac
Copy link
Contributor Author

adtac commented Dec 18, 2019

@dfawley now storing profiling data in the results file. I don't think I'll have time for this, but when diffing two result files with benchresult, we could also diff key areas within profiling if the data exists (i.e. -profiling=on). Like, time spent in the encoding layer has decreased by 7%, and so on.

@dfawley
Copy link
Member

dfawley commented Dec 18, 2019

@adtac, looks like this is still failing (not just flakes). Can you take another look please?

@adtac
Copy link
Contributor Author

adtac commented Dec 18, 2019 •
edited
Loading

@dfawley argh, forgot to run go fmt -- fixed, should be all green now (except for one documented xDS related failing test?)

@dfawley
Copy link
Member

dfawley commented Dec 18, 2019

Looks like there is a nil pointer dereference in ./test/TestCancelNoIO. Can you check that out please?

@adtac
Copy link
Contributor Author

adtac commented Dec 18, 2019

Weird, that didn't happen when I ran the tests locally...

Anyway, pushed one more update -- all green locally and on Travis.

@adtac adtac closed this Dec 18, 2019
@adtac adtac reopened this Dec 18, 2019
@adtac
Copy link
Contributor Author

adtac commented Dec 18, 2019

Closed by mistake lol

dfawley
dfawley reviewed Dec 19, 2019
View reviewed changes
benchmark/benchmain/main.go Outdated Show resolved Hide resolved
internal/transport/handler_server.go Show resolved Hide resolved
internal/transport/handler_server.go Outdated Show resolved Hide resolved
internal/transport/handler_server.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
adtac
adtac commented Dec 19, 2019
View reviewed changes
Copy link
Contributor Author

@adtac adtac left a comment

Choose a reason for hiding this comment

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

@dfawley addressed -- some of the TODOs surrounding recvMsg is from weeks ago when I used to pass a timer in that struct. That's no longer the case. There are still some legitimate TODOs left, which I've left in -- if you find some unnecessary TODOs, lemme know.

internal/transport/handler_server.go Show resolved Hide resolved
internal/transport/handler_server.go Outdated Show resolved Hide resolved
internal/transport/handler_server.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
@adtac adtac force-pushed the adtac/profiling-hooks branch 3 times, most recently from 66bc097 to 8c10159 Compare December 20, 2019 21:00
dfawley
dfawley reviewed Dec 20, 2019
View reviewed changes
internal/transport/http2_server.go Outdated
// See comment above StreamStatMetadataSize definition for more information
// on this encoding.
s.stat.Metadata = make([]byte, profiling.StreamStatMetadataSize)
binary.BigEndian.PutUint64(s.stat.Metadata[0:8], t.connectionID)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry: I was suggesting consts for both the 8 and 12 magic numbers (where 12 potentially = the 8 constant plus another 4 constant), and to use them here, also.

@dfawley
Copy link
Member

dfawley commented Dec 20, 2019

Travis:

+grep -vE '(_mock|\.pb)\.go:'
internal/profiling/profiling.go:226:2: comment on exported const StreamStatMetadataSize should be of the form "StreamStatMetadataSize ..."

@dfawley dfawley assigned menghanl and unassigned adtac Jan 6, 2020
@dfawley dfawley requested a review from menghanl January 6, 2020 21:22
menghanl
menghanl reviewed Jan 23, 2020
View reviewed changes
internal/transport/http2_server.go Outdated Show resolved Hide resolved
internal/transport/transport.go
@@ -454,26 +459,36 @@ func (s *Stream) write(m recvMsg) {
s.buf.put(m)
}

// Stat returns the streams's underlying *profiling.Stat.
func (s *Stream) Stat() *profiling.Stat {
if s == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need the nil check? Where would we call Stat() on a nil stream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't exactly remember, but I vaguely recall facing an issue in some end-to-end tests. I've added a TODO so that someone will re-evaluate this later.

@menghanl menghanl modified the milestones: 1.27 Release, 1.28 Release Jan 28, 2020
@dfawley dfawley assigned adtac and unassigned menghanl Jan 29, 2020
@adtac
Copy link
Contributor Author

adtac commented Feb 8, 2020

@menghanl fixed review comments.

@dfawley updated. See email.

@adtac
Copy link
Contributor Author

adtac commented Feb 8, 2020

@dfawley you (or someone) should also take over the 3-4 TODO comments I have in my name as TODO(adtac). I've added one more in this PR so that you know exactly which ones I intended to fix while I was there. I should probably also be unassigned from the issues I'm assigned to (except #1986 which should be closed once this PR is merged).

@menghanl
Copy link
Contributor

Thanks for the fixes! 🚀

@menghanl menghanl merged commit 83263d1 into grpc:master Feb 12, 2020
dfawley added a commit to dfawley/grpc-go that referenced this pull request Feb 13, 2020
@dfawley
Revert "profiling: add hooks within grpc (grpc#3159)"
1f021ad 
This reverts commit 83263d1.
dfawley added a commit that referenced this pull request Feb 14, 2020
@dfawley
Revert "profiling: add hooks within grpc (#3159)" (#3378)
6b9bf42 
This reverts commit 83263d1.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@menghanl menghanl menghanl approved these changes

@dfawley dfawley dfawley approved these changes

Assignees

@adtac adtac

Labels
Type: Performance Performance improvements (CPU, network, memory, etc)
Projects
None yet
Milestone
1.28 Release
Development

Successfully merging this pull request may close these issues.
3 participants
@adtac @dfawley @menghanl