Introduce CpuProfiler, CpuProfile, and CpuProfileNode #167
Conversation
- SamplesCount requires the intro of Sampler + StartSampleCount so will defer for now - GetAllProfiles, GetProfile, DeleteProfile on profiler TODO
Codecov Report
@@ Coverage Diff @@
## master #167 +/- ##
==========================================
+ Coverage 95.38% 95.78% +0.40%
==========================================
Files 12 16 +4
Lines 498 546 +48
==========================================
+ Hits 475 523 +48
Misses 14 14
Partials 9 9
Continue to review full report at Codecov.
|
|
Can you open an issue to add documentation around how to use these exposed types? |
There was a problem hiding this comment.
Hmm, I wonder if this is a good time to consider introducing some additional packages to expose different aspects of v8's api.
Having parity with the v8 namespace is certainly a nice quality but as we add more surface area it will make use of the package for newcomers more complicated. (I imagine most users won't initially leverage these types).
@genevieve mentioned that we could follow how v8 splits up header files (e.g. profiler, inspector).
@rogchap thoughts?
For this users, is this mostly a documentation problem? Note that this could easily introduce the opposite problem of making it harder for users to find functionality when it isn't clear which package it is in.
Note that there are 17 top-level header files in the version of v8 that v8go is currently using and v8 master now has In order to split the packages, we might also need an internal/unsafe package that could expose an V8 pointers that need to be accessed from other packages. |
genevieve
force-pushed
the
gl-cpuprofiler
branch
from
792c326 to
38767cb
Compare
genevieve
force-pushed
the
gl-cpuprofiler
branch
from
dcae810 to
bf396fb
Compare
pointers in StartProfiling/StopProfiling
cpuprofiler.go
Outdated
| startTime: timeUnixMicro(-int64(profile.startTime)), | ||
| endTime: timeUnixMicro(-int64(profile.endTime)), |
There was a problem hiding this comment.
The negation of the monotonic time values just seems wrong. I think the only reason why monotonic time values might be negative would be when these values are subtracted without knowing the ordering of the times to produce a possibly negative duration. The C standard unfortunately considering unsigned underflow to be defined behaviour, so the compiler won't even warn when it can detect this, so signed numbers may end up getting used to represent unsigned values.
It seems like this change was made to make the if cpuProfile.GetStartTime().Before(cpuProfile.GetEndTime()) { test pass, rather than using it as a sign that the test may be wrong.
| startTime: timeUnixMicro(-int64(profile.startTime)), | |
| endTime: timeUnixMicro(-int64(profile.endTime)), | |
| startTime: timeUnixMicro(int64(profile.startTime)), | |
| endTime: timeUnixMicro(int64(profile.endTime)), |
cpuprofile_test.go
Outdated
| t.Errorf("expected (root), but got %v", root.GetFunctionName()) | ||
| } | ||
|
|
||
| if cpuProfile.GetStartTime().Before(cpuProfile.GetEndTime()) { |
There was a problem hiding this comment.
We want start time to be before the end time, not to give a test error in that case.
| if cpuProfile.GetStartTime().Before(cpuProfile.GetEndTime()) { | |
| if !cpuProfile.GetStartTime().Before(cpuProfile.GetEndTime()) { |
This PR introduces the CpuProfiler, CpuProfile, and CpuProfileNode classes from v8. It originally started by introducing each class separately with their respective functions but it was pointed out that this would require a lot more cgo calls for a user to actually get the entire profile that they want. Instead, it now internally loads everything once a user calls
cpuProfiler.StopProfiling("profile-name")onto aCPUProfilego object which can be traversed by the user to print the relative details of the profile they want. This (1) speeds up the performance time by doing as much in c++ as possible and reducing the number of cgo calls that need to be made and (2) reduces the amount of code a user will need to write in order to get the entirety of the profile they are looking for.There's more to do like:
GetSampleso we can call it during our tests and ensure we have a sample to reduce flakiness)For now, this allows users to generate cpu profiles. Added some instructions to the readme to print the profile and followed a bit of the pattern from here for function name - script resource name - line number - column number formatting.
The default sampling interval is
1000 us = 1 millisecondand we seem to occasionally get profiles generated without the script. It's possible that the sampling is somehow happening before/after the script executes. One way to guarantee a sample has been taken for testing is to supportCollectSampleAPI on the profiler.