-
Notifications
You must be signed in to change notification settings - Fork 286
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
Implement pprof module extension #3773
Conversation
Test case 1 - Starting harmony without any arguments or configRunning: Result:
Problems Found: None Test case 2 - Passing --pprof --pprof.folder ./profiles --pprof.profile.names cpu,heap,goroutine --pprof.profile.intervals 10 --pprof.profile.debug 0,0,2Running:
Result:
Problems Found: None Test case 3 Running with config 1.0.2Running:
Result:
Problems Found: None Test case 4 Running with config 1.0.3Running:
Result:
Problems Found: None Test case 5 Running with config 1.0.4Running:
Result:
Problems Found: None |
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.
Sorry for the late reply. Some suggestions in the comments. Please fix. Thanks.
Please rebase and solve the conflict introduced by another config upgrade. Thanks!! |
Thank your for the review. Really good points indeed! I will try to resolve them asap. |
Setting the default value in config_migrations.go as follows results in the error
Changing Do you know what I'm missing here @JackyWYX ? |
Changing |
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.
Still some comments out there. Please fix. Thanks!
Ready for round 3 @JackyWYX |
api/service/pprof/service.go
Outdated
} | ||
svc.profiles = profiles | ||
|
||
go func() { |
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.
Do you mind moving these two lines to service.Start
?
// stopCpuProfile stops the current CPU profile, if any | ||
func stopCpuProfile() { | ||
pprof.StopCPUProfile() | ||
if cpuFile != nil { |
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.
Have you tried testing with go test --race
of this line? It seems there is a data race condition of cpuFile
.
The race condition can be detected by
- implement a unit test with
service.Start
and last for at least one iteration. - Test with
go test --race
.
One last small comments about data racing. |
Fixed data racing using a mutex and moved the two lines @JackyWYX |
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.
Thank you @JackyWYX, that review process was rigorous. :) Makes me appreciate the harmony project even more. With that, do you consider this bounty as completed? The deadline is approaching quite soon (~4 days). |
Please resolve the last comment about the variable name |
looks good to merge if this is backward-compatible. We should do more testing to make sure no race condition and it won't crash the node. |
What kind of testing are you suggesting? |
Like extended period of testing longevity test. Like doing frequent pprof every 1s for 2 hours and see if there is any race condition and other problems. |
Sound reasonable. @niklr Could you please do this test and give out the machine performance metrics? (Like CPU usage, memory, with / without pprof module) |
Sure, I will record metrics with the following script tomorrow (getting late here already):
Or do you have any other preferences @JackyWYX ? |
Could you do performance benchmark on this PR and previous code to see the performance difference? |
10 minutes running the previous code + 2 hours running this PR with pprof enabled (--pprof.profile.names cpu,heap --pprof.profile.intervals 1) Something like that? |
Can you do 2 hours running previous code vs 2 hours running with pprof enabled? |
Harmony performance profiling over 2 hours |
Issue
@JackyWYX
harmony-one/bounties#46
Test
Unit Test Coverage
go test -cover ./cmd/harmony/
Before:
After:
Test/Run Logs
When running and stdin is terminal
When stdin is piped
Operational Checklist
NO
NO
Manual / cheatsheet
pprof basics
pprof lets you collect CPU profiles, traces, and heap profiles. The normal way to use pprof is:
./harmony --pprof
curl localhost:6060/debug/pprof/{profile_name} --output profile.pb.gz
to save a profilego tool pprof profile.pb.gz
to analyze said profilepprof profiles
A Profile is a collection of stack traces showing the call sequences that led to instances of a particular event, such as allocation. Packages can create and maintain their own profiles; the most common use is for tracking resources that must be explicitly closed, such as files or network connections.
Each Profile has a unique name. A few profiles are predefined:
For more information see runtime/pprof docs
A special profile accepted by
--pprof.profile.names
pprof config
Example 1
./harmony --pprof.profile.names cpu,heap --pprof.profile.intervals 0,10
Saves the cpu and heap profiles in the
./profiles
folder. The cpu profile will be saved on shutdown and the heap profile will be saved every 10 seconds.Example 2
./harmony --pprof.profile.names cpu,heap,goroutine --pprof.profile.intervals 10 --pprof.profile.debug 0,0,2
Saves the cpu, heap and goroutine profiles in the
./profiles
folder. All profiles will be saved every 10 seconds.The goroutine profile will use debug=2 which means to print the goroutine stacks in the same form that a Go program uses when dying due to an unrecovered panic.