profiler: wrap delta profiling in a type #1483
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nsrip-dd
commented
Sep 27, 2022
felixge
previously approved these changes
Sep 29, 2022
profiler/profile.go
Outdated
| // profile after computing the delta profile. | ||
| func (p *profiler) deltaProfile(name string, delta *pprofutils.Delta, curData []byte, extra ...*pprofile.Profile) (*profile, error) { | ||
| type deltaProfiler struct { | ||
| delta *pprofutils.Delta |
There was a problem hiding this comment.
NIT: I think I'd prefer the delta config to be an argument to the deltaProfiler.Delta() function. Otherwise it's kind of odd to have two copies of the same *pprofutils.Delta with different pointer semantic in two different places (here and on the profile type struct). The pointer here is meaningless (could be removed), and the pointer on the profile type has an overloaded meaning to indicate that delta profiling is not being used.
Again, I think it would all be a bit clearer if there weren't two copies of this type with the same data.
It's still fine to have a type that abstract the state of the delta profiler rather than directly using *pprofile.Profile to prepare for the upcoming change.
There was a problem hiding this comment.
Per our discussion, I've done the following things:
- Make the
Deltafield forprofileTypea slice ofpprofutils.ValueTypes and rename it toDeltaValues. - Change
pprofutils.Deltaso that it only computes deltas for the values it was explicitly given. This way, a length 0pprofutils.ValueTypemeans "don't compute delta profiles" so we don't overload the meaning of theDeltafield. - Make the
deltafield ofdeltaProfilera value rather than a pointer.
Wrap up delta profiling in a type with a Delta method. This type holds any information about the previous profile needed to compute the delta with the newest profile. The profiler keeps an instance of type for each profile type which supports delta profiling. This better encapsulates the implementation details of delta profiling, and facilitates upcoming implementation changes. As part of this change, pull the logic for merging in "extra" profiles out of the delta profiling code path. This logic was implemented for C allocation profiling, and the extra data was passed through to delta profiling mainly for efficiency. However, merging in extra data is orthogonal to delta profiling and we can find other ways to make that more efficient if we need to.
Push more of the implementation details into the deltaProfiler type
Change the Delta field of profileType to DeltaValue, a list of values for which to compute deltas. A length-0 DeltaValue means that the profile type doesn't support delta profiles. This gets rid of the overloaded meaning of Delta == nil meaning "do nothing" and len(Delta.SampleTypes) == 0 mean "do everything".
nsrip-dd
force-pushed
the
nick.ripley/delta-profile-type
branch
from
79e10c9
to
d796776
Compare
pmbauer
approved these changes
Sep 30, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Wrap up delta profiling in a type with a
Deltamethod. This type holdsany information about the previous profile needed to compute the delta
with the newest profile. The profiler keeps an instance of type for each
profile type which supports delta profiling. This better encapsulates
the implementation details of delta profiling, and facilitates upcoming
implementation changes.
As part of this change, pull the logic for merging in "extra" profiles
out of the delta profiling code path. This logic was implemented for C
allocation profiling, and the extra data was passed through to delta
profiling mainly for efficiency. However, merging in extra data is
orthogonal to delta profiling and we can find other ways to make that
more efficient if we need to.