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

For PProf.Allocs, change the default metric to allocs, not size. #55

Merged
merged 1 commit into from
Feb 16, 2022

Conversation

NHDaly
Copy link
Member

@NHDaly NHDaly commented Feb 15, 2022

We default to allocs, since julia's Profile.Allocs code currently
uniformly samples accross allocations, so allocs is a representative
profile, while size is not.

Explanation:

If each allocation was uniform (as in the case of the allocs sample
type), then uniformly sampling should give an accurate distribution of
where the allocations occur: code that allocates more often is more
likely to show up in the profile.

But if the metric we're interested in is not uniform: how many bytes are
allocated, uniform sampling will not get us a representative
distribution of where the bytes are allocated! Code that allocates very
large objects is no more or less likely to be included than code that
allocates very small ones.

We default to allocs, since julia's Profile.Allocs code currently
uniformly samples accross allocations, so allocs is a representative
profile, while size is not.

Explanation:

If each allocation was uniform (as in the case of the allocs sample
type), then uniformly sampling should give an accurate distribution of
where the allocations occur: code that allocates more often is more
likely to show up in the profile.

But if the metric we're interested in is not uniform: how many bytes are
allocated, uniform sampling will not get us a representative
distribution of where the bytes are allocated! Code that allocates very
large objects is no more or less likely to be included than code that
allocates very small ones.
@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2022

Codecov Report

Merging #55 (003440b) into master (18d842d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #55   +/-   ##
=======================================
  Coverage   72.81%   72.81%           
=======================================
  Files           3        3           
  Lines         298      298           
=======================================
  Hits          217      217           
  Misses         81       81           
Impacted Files Coverage Δ
src/Allocs.jl 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18d842d...003440b. Read the comment docs.

@NHDaly NHDaly marked this pull request as ready for review February 16, 2022 14:28
@NHDaly NHDaly merged commit 4bff9e7 into master Feb 16, 2022
@NHDaly NHDaly deleted the nhd-change-pprof-allocs-default-to-allocs-metric branch February 16, 2022 14:28
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 this pull request may close these issues.

2 participants