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

Poor use of memory / too many allocations in HashLeaf, leading to GC to work in overdrive #216

Open
musalbas opened this issue Jun 28, 2023 · 5 comments

Comments

@musalbas
Copy link
Member

musalbas commented Jun 28, 2023

More time is spent in HashLeaf allocating, than hashing:

image
https://github.com/celestiaorg/nmt/blob/master/hasher.go#L190
https://flamegraph.com/share/a54e891a-1114-11ee-b13f-de9431916b05


Possibly related:

In celestia-node, min and max namespace take up 200MB (20%) of RAM

image

@musalbas musalbas changed the title Poor use of memory / too many allocations for nmt.MinNamespace and nmt.MaxNamespace, leading to GC to work in overdrive Poor use of memory / too many allocations, leading to GC to work in overdrive Jun 28, 2023
@musalbas
Copy link
Member Author

If the same allocation is reused in HashLeaf--would that make it not threadsafe? If so, should be considered, I believe trees are often computed in parallel.

@musalbas musalbas changed the title Poor use of memory / too many allocations, leading to GC to work in overdrive Poor use of memory / too many allocations in HashLeaf, leading to GC to work in overdrive Jun 28, 2023
@liamsi
Copy link
Member

liamsi commented Jun 28, 2023

I would try the folllwing fwiw. Change these appends

nmt/hasher.go

Lines 190 to 192 in 564300a

minMaxNIDs := make([]byte, 0, resLen)
minMaxNIDs = append(minMaxNIDs, nID...) // nID
minMaxNIDs = append(minMaxNIDs, nID...) // nID || nID

to

minMaxNIDs :=  append(append(make([]byte, 0, resLen), nID...), nID...) 

and see if that removes the need for additional allocs. Similar for the other appends:

nmt/hasher.go

Lines 195 to 197 in 564300a

leafPrefixedNData := make([]byte, 0, len(ndata)+1)
leafPrefixedNData = append(leafPrefixedNData, LeafPrefix)
leafPrefixedNData = append(leafPrefixedNData, ndata...)

@liamsi
Copy link
Member

liamsi commented Jun 28, 2023

Also, it is important to use benchstat for any related changes (since we disabled go-bencher recently): https://pkg.go.dev/golang.org/x/perf/cmd/benchstat

Should include both mem and CPU.

@liamsi
Copy link
Member

liamsi commented Jun 28, 2023

This could be somewhat related: #212

@staheri14
Copy link
Contributor

Unassigning myself to allow someone else to take over.

@staheri14 staheri14 removed their assignment Dec 2, 2024
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

No branches or pull requests

3 participants