@seankhliao would it make sense to add the label "proposal" for this? I'm afraid it might not be reviewed otherwise.

cc @hyangah

Thank you for the detailed explanation and writeup.

cc @dvyukov (blockprofile author), @pjweinb (mutexprofile author), @rsc (heap profile author) - all three profiles share similar sampling logic.

Improving sampling bias sounds great. I have very limited experience with blockprofile so I want to learn more about the goal here. In your motivating example scenario, why is it important to present the two kinds of events equally? If one type of events usually block 10x longer on average, isn't it more interesting and worth more attention?

@hyangah thx.

all three profiles share similar sampling logic.

It's similar, but I think all three of them use slightly different approaches:

• Heap Profiler: Uses an internal counter to accumulate bytes allocated. Any event causing the counter to exceed MemProfileRate gets sampled and resets the counter. Very simplified explanation, see runtime/malloc.go for full details.
• Block profiler: Samples events with a probability of min(1, duration/rate). In code review @dvyukov mentioned that this was intended to be similar to MemProfileRate.
• Mutex Profiler: The simplest sampling implementation. It doesn't take into account the event duration at all, and just samples events with a probability of 1/rate.

Improving sampling bias sounds great. I have very limited experience with blockprofile so I want to learn more about the goal here. In your motivating example scenario, why is it important to present the two kinds of events equally? If one type of events usually block 10x longer on average, isn't it more interesting and worth more attention?

Great point. I think it depends : ). I could imagine a scenario where you have two producers sending work into two separate channels, sometimes getting blocked while doing so. I'd expect the block profiler to be able to tell me which of those two producers is spending more time blocked than the other. Given the current bias, it could give me the wrong answer to that question.

Or from another point of view, imagine if the heap profiler was biased towards big allocations (i hope it isn't, still need to analyze it some more). It could make you chase a few big allocation sources even so they may contribute less overall garbage than thousands of small allocations that the profiler is telling you to ignore by under-representing them.

Anyway, I'd be okay if the bias is here to stay. But if it is, I think it's worth documenting, which I'd be happy to send patches for.

Interesting. I can see how this change makes sense:
felixge/go@master...debias-blockprofile-rate#diff-4af3428aab976e43f655b4e1a35c92a608a92294609190aa42378d2d265d501cR414

Have you looked at what's done in tcmalloc?
https://github.com/google/tcmalloc/blob/master/tcmalloc/sampler.cc#L168-L177
The logic there was given a lot of thought and it passed several rounds of de-biasing already :)
At least for heap sampling it would be good to mimic exactly the same logic I think. Maybe for other samplers as well.

Interesting. I can see how this change makes sense:

Cool : ). Do you think the fact that it obscures the mean of the samples (total_delay / count) is a problem? It's kind of a nice thing to be able to compute, but OTOH I'm not sure if anybody ever looks at it.

Have you looked at what's done in tcmalloc?

Not yet, thanks for the link!

At least for heap sampling it would be good to mimic exactly the same logic I think. Maybe for other samplers as well.

If the current heap sampling could be improved, that'd be awesome. But I'll need some time to fully wrap my head around the current Go implementation as well as tcmalloc's sampler before I could contribute anything useful to that discussion. If you're aware of any good design documents or literature discussing heap sampling techniques, please let me know.

Do you think the fact that it obscures the mean of the samples (total_delay / count) is a problem?

Lie in profiles is definitely bad. If I see any numbers that definitely look like a lie I may throw away whole profile b/c I don't know what I can trust there.
Humm.... could we make blockRecord.count a float and record real number of cycles and rate/cycles samples? It should fix the total, but preserve average.

If you're aware of any good design documents or literature discussing heap sampling techniques, please let me know.

I am not aware of anything besides comments in sampler.h:
https://github.com/google/tcmalloc/blob/master/tcmalloc/sampler.h#L33

Lie in profiles is definitely bad. If I see any numbers that definitely look like a lie I may throw away whole profile b/c I don't know what I can trust there.

👍 I love this perspective / mindset.

Humm.... could we make blockRecord.count a float and record real number of cycles and rate/cycles samples? It should fix the total, but preserve average.

This sounds interesting, but I don't think I understand it. Can you explain it differently?

Humm.... could we make blockRecord.count a float and record real number of cycles and rate/cycles samples? It should fix the total, but preserve average.

This sounds interesting, but I don't think I understand it. Can you explain it differently?

In saveblockevent we do:

func saveblockevent(cycles int64, skip int, which bucketType) {
	...
	b.bp().count++
	b.bp().cycles += cycles
	...
}

You change increases cycles, but count is still incremented by 1. This makes average incorrect.
My idea was to do something like:

	b.bp().count += rate/cycles
	b.bp().cycles += rate

This increases cycles, but also proportionally increases count.

Edit: I updated this comment, you can still see the original below, but I've hidden it because it was confusing.

rate = 100
20 events with 500 cycles each
--->
count = 4 (100/500*20)
cycles = 2000 (1000*20)
mean = 500 (2000/40

rate = 100
10 events with 1000 cycles each
--->
count = 1 (100/1000*10)
cycles = 1000 (100*10)
mean = 1000 (1000/1)

  b.bp().count += rate/cycles
  b.bp().cycles += rate

@dvyukov it took me a bit to figure out if you were suggesting this change in addition to my patch or instead of it. Neither combination seemed to work. However, I think I finally figured out that your idea could be applied by a new patch that simply does this inside of saveblockevent. At least in my testing it seems to fix the bias and produce a sample mean that converges on the population mean.

if cycles < rate {
	b.bp().count += rate/cycles
	b.bp().cycles += rate
} else {
	b.bp().count += 1
	b.bp().cycles += cycles
}

I'll think this over again tomorrow when it's not late, but I'd be happy to send a patch + tests for this if you agree. Let me know.

Yes, I meant in combination with your patch. Something similar to the snippet you posted. I just did not elaborate enough, sorry.

Yes, I meant in combination with your patch. Something similar to the snippet you posted. I just did not elaborate enough, sorry.

No worries, in retrospect I think it was clear enough, but my brain didn't want to collaborate 🙈. I'll try to prepare a patch for this now to continue the discussion.

I see Heap profiler's sampling is more similar to what tcmalloc sampler is doing - having an internal counter to keep track of the next sampling point instead of tossing a coin every time. Is it an option to change the block profile sampler to be more similar to what the current heap profiler or tcmalloc sampler are doing?

@hyangah I think the main concern there would be doing this in a way that doesn't fundamentally impact the overhead of block profiling or change the semantics of the blockprofilerate in a way that will lead to less accurate profiles. But if there is consensus that this would be a better direction, I'd be happy to give it a shot.

Meanwhile I got a patch ready based on @dvyukov's suggestions here: felixge@a3dc518

But I'll hold off on submitting it for now. I also still have to sort out some CLA stuff on my end.

Change https://golang.org/cl/299991 mentions this issue: runtime/pprof: fix block profile bias

@hyangah @dvyukov I just submitted my current patch. I think it's a decent improvement for now.

However, I'd also be happy to look into aligning the sampling with heap/tcmalloc in a follow-up patch if that's something you're interested in.