perf: use nmt buffering for root calcuation#381
Conversation
Summary of ChangesHello @mcrakhman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the performance of NMT root calculations within the data square by introducing a buffered NMT pooling mechanism. This change aims to minimize memory allocations and optimize concurrent processing, leading to more efficient extended data square computations. The new implementation includes a configurable parallelism limit and is thoroughly tested for correctness and performance benefits. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a buffering and pooling mechanism for NMT root calculation to improve performance by reducing memory allocations. This is achieved through a treePool of resizeableBufferTrees, with concurrency managed by a new parallelOps setting. The changes are logical and supported by new benchmarks and correctness tests that validate the buffered approach against the existing implementation.
I've found one critical issue and one of medium severity:
- The new file
nmtbuffered_tree_test.gocontains production code but is named as a test file, which will cause build issues. - There's a redundant check in the
Pushmethod ofresizeableBufferTree.
Details are in the review comments below. Once these are addressed, this looks like a solid performance improvement.
| if nidDataLen > t.bufferEntrySize { | ||
| return fmt.Errorf("data is too large to be used with allocated buffer") | ||
| } |
There was a problem hiding this comment.
This check is redundant and can be removed. nidDataLen is calculated as t.namespaceSize + len(data), and t.bufferEntrySize is t.namespaceSize + shareSize. The newDataSquare function already ensures that len(data) is equal to shareSize for all non-nil shares. Therefore, nidDataLen will always be equal to t.bufferEntrySize, and this condition will never be met.
f9a870d
Overview
Use buffering approach during root calculation:
This gives 95% lower allocations for subsequent reuses and 95% less memory usage and 2x speed.
Note: the pr relies on improved NMT version which supports buffer reuse for hashes: celestiaorg/nmt#305
Benchmarks
Without buffering
With buffering