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.

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 percentage calculation in diff mode (-base) and remove -positive_percentages #323

Closed
nolanmar511 opened this issue Feb 15, 2018 · 0 comments
Assignees

Comments

@nolanmar511
Copy link
Contributor

nolanmar511 commented Feb 15, 2018

Right now, the denominator used when computing percentages when there is a base profile is the sum of the absolute value for all samples in the merged profile (which is source profile merged with negative base profile), before samples are aggregated by function, line number, file, etc. Usually, this results in the denominator used when computing percentages being the sum of the total of samples in the source profile(s) and sum of the total of samples in the base profile, because the addresses for samples are generally different. However, this is not always the case; for example, when the source and base profile are identical, the denominator used would be zero.

This is useful when looking at cumulative profiles, but not useful when using the -base flag to compare profiles. So, we intend to add a new option -diff_base, which would be used similarly to -base, but compute the denominator for percentages in a way which is more useful when comparing profiles.

We want to make the denominator used when computing percentages when there is a diff base profile equal to the sum of all samples in the diff base profile. There are several advantages to this over the current method. Most notably, the denominator will be consistent, regardless of the address space of the profiles being compared. Also, percentages when comparing two different source profiles to the same base profile will be comparable, since the same denominator was used to compute both.

To do this, prior to merging the source and base profiles, a tag pprof::base with a value "true" will be added to each sample in the diff base profile. Then, when computing the denominator, we will return the absolute value of the sum of all profiles with the pprof::base tag if this tag is present in the profile; otherwise we will return the sum of the absolute value of all samples (which is currently done for all profiles). There are several advantages to this approach for computing the denominator. First, it will be a relatively small code change compared to other methods considered (for example, we considered maintaining a separate copy of the base profile through most steps which pprof does). Next, it provides some consistency when using a merged profile with a diff base -- if this profile is written out, the samples from the diff base profile will still have the pprof::diff tag, and so if pprof is used to inspect that profile, it will still compute the denominator as the sum of samples in the base profile.

Since this change means that the denominator used when there is a base profile will no longer generally be the sum of positive sample from the source profile and the absolute value of negative samples from the base profile, we are also removing the -positive_percentages flag, which allowed users to choose to only include positive samples when computing the denominator. This change makes this flag somewhat unnecessary -- since -positive_percentages was generally used to make the denominator equal to the sum of samples in one profile, which is very similar to what -diff_base does. This flag would also be problematic if used with the method for computing the denominator outline above, since, if it were used when computing the denominator for a merged profile as described above, the denominator would 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants