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.

Sign up for GitHub

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

Jump to bottom

test: extends computeRoots benchmark to cover larger square sizes and nmt tree #304

Merged
merged 11 commits into from
Mar 7, 2024
Merged

test: extends computeRoots benchmark to cover larger square sizes and nmt tree #304

merged 11 commits into from
Mar 7, 2024

Conversation

staheri14
Copy link
Contributor

Closes #303

@staheri14
Copy link
Contributor Author

Results obtained are as follows, for a 512MB block size (ODS) the computeRoots takes roughly 1 seconds.

BenchmarkEDSRoots
BenchmarkEDSRoots/32x32x512_ODS=0MB,_EDS=2MB
BenchmarkEDSRoots/32x32x512_ODS=0MB,_EDS=2MB-8         	    1028	   1266168 ns/op
BenchmarkEDSRoots/64x64x512_ODS=2MB,_EDS=8MB
BenchmarkEDSRoots/64x64x512_ODS=2MB,_EDS=8MB-8         	     100	  10194347 ns/op
BenchmarkEDSRoots/128x128x512_ODS=8MB,_EDS=32MB
BenchmarkEDSRoots/128x128x512_ODS=8MB,_EDS=32MB-8      	      76	  19787965 ns/op
BenchmarkEDSRoots/256x256x512_ODS=32MB,_EDS=128MB
BenchmarkEDSRoots/256x256x512_ODS=32MB,_EDS=128MB-8    	      22	  65517998 ns/op
BenchmarkEDSRoots/512x512x512_ODS=128MB,_EDS=512MB
BenchmarkEDSRoots/512x512x512_ODS=128MB,_EDS=512MB-8   	       5	 210380242 ns/op
BenchmarkEDSRoots/1024x1024x512_ODS=512MB,_EDS=2048MB
BenchmarkEDSRoots/1024x1024x512_ODS=512MB,_EDS=2048MB-8         	       1	1034866750 ns/op

@staheri14 staheri14 self-assigned this Mar 5, 2024
@Wondertan
Copy link
Member

Hmm, this benchmark uses the default tree, while profiles on the issue are based on NMT

@staheri14 staheri14 marked this pull request as draft March 5, 2024 18:23
@staheri14
Copy link
Contributor Author

I updated the tree to ErasuredNamespacedMerkleTree and below are the updated results:

BenchmarkEDSRootsWithErasuredNMT
BenchmarkEDSRootsWithErasuredNMT/32x32x512_ODS=0MB,_EDS=2MB
BenchmarkEDSRootsWithErasuredNMT/32x32x512_ODS=0MB,_EDS=2MB-8         	     262	   4435680 ns/op
BenchmarkEDSRootsWithErasuredNMT/64x64x512_ODS=2MB,_EDS=8MB
BenchmarkEDSRootsWithErasuredNMT/64x64x512_ODS=2MB,_EDS=8MB-8         	      84	  14465517 ns/op
BenchmarkEDSRootsWithErasuredNMT/128x128x512_ODS=8MB,_EDS=32MB
BenchmarkEDSRootsWithErasuredNMT/128x128x512_ODS=8MB,_EDS=32MB-8      	      20	  54858510 ns/op
BenchmarkEDSRootsWithErasuredNMT/256x256x512_ODS=32MB,_EDS=128MB
BenchmarkEDSRootsWithErasuredNMT/256x256x512_ODS=32MB,_EDS=128MB-8    	       5	 214332758 ns/op
BenchmarkEDSRootsWithErasuredNMT/512x512x512_ODS=128MB,_EDS=512MB
BenchmarkEDSRootsWithErasuredNMT/512x512x512_ODS=128MB,_EDS=512MB-8   	       2	 826557604 ns/op
BenchmarkEDSRootsWithErasuredNMT/1024x1024x512_ODS=512MB,_EDS=2048MB
BenchmarkEDSRootsWithErasuredNMT/1024x1024x512_ODS=512MB,_EDS=2048MB-8         	       1	4659166292 ns/op

Up to 128 MB of ODS, the computeRoots takes less than a second, but for 512MB of ODS this value goes to5seconds.

@staheri14 staheri14 marked this pull request as ready for review March 5, 2024 23:41
rootulp
rootulp reviewed Mar 6, 2024
View reviewed changes
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

The benchmark in the PR comments LGTM. Left a few comments about readability.

datasquare_test.go Outdated Show resolved Hide resolved
datasquare_test.go Outdated Show resolved Hide resolved
datasquare_test.go Outdated Show resolved Hide resolved
datasquare_test.go Outdated
Comment on lines 453 to 454
i*i*shareSize/(1024*1024),
2*i*2*i*shareSize/(1024*1024)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm kind of lost in the math here. Would it help readability to extract variables with names for these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added more vars and clarifying comments in 9b26deb

@staheri14 staheri14 changed the title test: extends computeRoots benchmark for larger square sizes test: extends computeRoots benchmark to cover larger square sizes and nmt tree Mar 6, 2024
@staheri14 staheri14 requested a review from rootulp March 6, 2024 20:31
rootulp
rootulp previously approved these changes Mar 6, 2024
View reviewed changes
datasquare_test.go Outdated Show resolved Hide resolved
datasquare_test.go Outdated Show resolved Hide resolved
datasquare_test.go Outdated Show resolved Hide resolved
@staheri14 staheri14 requested a review from rootulp March 6, 2024 23:53
evan-forbes
evan-forbes approved these changes Mar 7, 2024
View reviewed changes
nmtwrapper_test.go
// calculate the data root. It creates that tree using the
// erasuredNamespacedMerkleTree.
func newConstructor(squareSize uint64, opts ...nmt.Option) TreeConstructorFn {
func newErasuredNamespacedMerkleTreeConstructor(squareSize uint64, opts ...nmt.Option) TreeConstructorFn {
Copy link
Member

Choose a reason for hiding this comment

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

a bit unrelated to this PR, but do we have the wrapper in both places, here and in app? https://github.com/celestiaorg/celestia-app/tree/main/pkg/wrapper

Copy link
Contributor Author

@staheri14 staheri14 Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good question! Yes, we have, I did it in the past (inside the nmtwrapper_test file) to conduct some tests in the rsmt2d side without making circular dependency to the app.
PS: There is also an open issue #248 about moving the wrapper completely to the rsmt2d repo.

@staheri14 staheri14 merged commit 86b809c into main Mar 7, 2024
5 checks passed
@staheri14 staheri14 deleted the sanaz/extending-computeRoots-benchmark branch March 7, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-forbes evan-forbes evan-forbes approved these changes

@rootulp rootulp rootulp approved these changes

@cmwaters cmwaters Awaiting requested review from cmwaters

Assignees

@staheri14 staheri14

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Benchmark computeRoots function for large square sizes
4 participants
@staheri14 @Wondertan @rootulp @evan-forbes