chore(perf): using tags instead of runtime callers to improve the performance of leak detection #255
Conversation
aman-bansal
requested review from
jarifibrahim,
manishrjain and
martinmr
as code owners
|
Not sure if this is going to be much faster than what we're doing today. |
aman-bansal
changed the title
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @aman-bansal, @jarifibrahim, @manishrjain, and @martinmr)
z/out.txt, line 0 at r2 (raw file):
Why do we have this file?
|
Shouldn't we run some numbers if the perf actually improves? Or is it in some accompanying PR? |
There was a problem hiding this comment.
Got some comments.
Reviewable status: 0 of 11 files reviewed, 8 unresolved discussions (waiting on @aman-bansal, @manishrjain, and @martinmr)
z/allocator.go, line 69 at r3 (raw file):
// NewAllocator creates an allocator starting with the given size. func NewAllocator(sz int, tag string) *Allocator {
a.Tag is already exposed. We'll need to fix all occurrences of NewAllocator if you make this change.
z/allocator.go, line 328 at r3 (raw file):
} func (p *AllocatorPool) Get(sz int, tag string) *Allocator {
Don't need this. a.Tag is already exposed.
z/buffer.go, line 76 at r3 (raw file):
// NewBuffer is a helper utility, which creates a virtually unlimited Buffer in UseCalloc mode. func NewBuffer(sz int, tag string) *Buffer {
If you just expose buffer.Tag you won't need this change and the rest of the changes in this file.
z/calloc_jemalloc.go, line 56 at r3 (raw file):
} func Calloc(n int, tag string) []byte {
I would've put tag before the size but that's a personal preference.
z/calloc_jemalloc.go, line 82 at r3 (raw file):
uptr := unsafe.Pointer(ptr) if dallocs != nil {
Remove this if
z/calloc_jemalloc.go, line 83 at r3 (raw file):
uptr := unsafe.Pointer(ptr) if dallocs != nil { // If leak detection is enabled.
Remove this comment
z/calloc_jemalloc.go, line 109 at r3 (raw file):
atomic.AddInt64(&numBytes, -int64(sz)) if dallocs != nil {
Remove this if
There was a problem hiding this comment.
Reviewable status: 0 of 11 files reviewed, 8 unresolved discussions (waiting on @ahsanbarkati, @aman-bansal, @jarifibrahim, @manishrjain, and @martinmr)
z/allocator.go, line 69 at r3 (raw file):
Previously, jarifibrahim (Ibrahim Jarif) wrote…
a.Tag is already exposed. We'll need to fix all occurrences of
NewAllocatorif you make this change.
The idea is if we make this part of allocator function than this will ensure that all new allocators will have tags. this is to make necessary that all tags have tags
z/buffer.go, line 76 at r3 (raw file):
Previously, jarifibrahim (Ibrahim Jarif) wrote…
If you just expose
buffer.Tagyou won't need this change and the rest of the changes in this file.
But in future use, it might happen that not all buffers will have tags. this is to make tags necessary
z/calloc_jemalloc.go, line 56 at r3 (raw file):
Previously, jarifibrahim (Ibrahim Jarif) wrote…
I would've put
tagbefore the size but that's a personal preference.
size seems imp. thats why 😅
z/calloc_jemalloc.go, line 82 at r3 (raw file):
Previously, jarifibrahim (Ibrahim Jarif) wrote…
Remove this if
Done.
z/calloc_jemalloc.go, line 83 at r3 (raw file):
Previously, jarifibrahim (Ibrahim Jarif) wrote…
Remove this comment
Done.
z/calloc_jemalloc.go, line 109 at r3 (raw file):
Previously, jarifibrahim (Ibrahim Jarif) wrote…
Remove this if
Done.
z/out.txt, line at r2 (raw file):
Previously, ahsanbarkati (Ahsan Barkati) wrote…
Why do we have this file?
missed it. deleted now
There was a problem hiding this comment.
Reviewable status: 0 of 11 files reviewed, 5 unresolved discussions (waiting on @ahsanbarkati, @aman-bansal, @jarifibrahim, @manishrjain, and @martinmr)
Earlier we used to loop over the caller function stack to find out who requested the memory. In this way it leads to multiple runtime.Caller calls that are expensive. Internally, runtime.Caller collects the full stack trace and then resolve the current callee function. We skipped this loop and get all the information(limit of 50, we can make this small though). Then we check for non-ristretto function among the complete stacktrace thus saving multiple runtime.Callers call. This should enhance the performance of calloc directly in the ratio of depth we have in the Calloc stack trace for the non-ristretto function.
This change is