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

pkg/storage: Remove storing cumulative values #337

Merged
merged 15 commits into from
Oct 20, 2021

Conversation

metalmatze
Copy link
Member

Instead of storing cumulative values, we are going to only store the flat values (these are the values that the leaves have), and then when generating the flame graph we'll calculate the cumulative values on the fly.

This will mostly show in heap usage over time:
heap

Benchmarks for appending got faster and more lightweight as well as querying (although we calculate all cumulative values).

Appends:

name        old time/op    new time/op    delta
Appends-24    16.5ms ± 1%    12.5ms ± 3%   ~     (p=0.100 n=3+3)

name        old alloc/op   new alloc/op   delta
Appends-24    7.24MB ± 0%    6.26MB ± 0%   ~     (p=0.100 n=3+3)

name        old allocs/op  new allocs/op  delta
Appends-24      152k ± 0%      122k ± 0%   ~     (p=0.100 n=3+3)

Iterator:

name         old time/op    new time/op    delta
Iterator-24     144µs ± 1%     104µs ± 5%   ~     (p=0.100 n=3+3)

name         old alloc/op   new alloc/op   delta
Iterator-24    28.7kB ± 0%    28.0kB ± 0%   ~     (p=0.100 n=3+3)

name         old allocs/op  new allocs/op  delta
Iterator-24     1.71k ± 0%     1.70k ± 0%   ~     (p=0.100 n=3+3)

paulfantom added a commit to thaum-xyz/ankhmorpork that referenced this pull request Oct 19, 2021
cumulative: []*ProfileTreeValueNode{{
Value: int64(21),
}},
//cumulative: []*ProfileTreeValueNode{{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why comment, not remove?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through all of these comments and removed them. Seems like I missed a few.
Thanks!

@@ -57,7 +58,7 @@ func TestMemRootSeries_Iterator(t *testing.T) {
itt := p.ProfileTree().Iterator()
for itt.HasMore() {
if itt.NextChild() {
require.Equal(t, seen, itt.At().CumulativeValues()[0].Value)
//require.Equal(t, seen, itt.At().CumulativeValues()[0].Value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love that we can show here that pre-computing the values actually made things slower. Architecturally looks good, and our regression tests should have caught any inconsistencies in results, so I feel confident that this works as expected!

I don't feel too strongly about the commented-out parts being removed in this or a follow-up PR. Your call.

@metalmatze
Copy link
Member Author

Awesome. Yeah, it's probably to go with this now and do the cleanup in a follow-up. :)
Thanks for the reviews!

@metalmatze metalmatze merged commit fb368a6 into main Oct 20, 2021
@metalmatze metalmatze deleted the storage-cumulative-removal branch October 20, 2021 10:48
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.

3 participants