Update tail padded shares with NMT wrapper #440
Conversation
…tail padding and not in wrapper; -1 allocation
Codecov Report
@@ Coverage Diff @@
## master #440 +/- ##
==========================================
- Coverage 61.76% 61.70% -0.06%
==========================================
Files 263 263
Lines 23003 23010 +7
==========================================
- Hits 14207 14198 -9
- Misses 7292 7303 +11
- Partials 1504 1509 +5
|
types/share_splitting.go
Outdated
| shares[i] = NamespacedShare{ | ||
| Share: append( | ||
| append(make([]byte, 0, shareWidth), consts.TailPaddingNamespaceID...), | ||
| bytes.Repeat([]byte{0}, shareWidth-consts.NamespaceSize)..., |
There was a problem hiding this comment.
Would it make sense to move the bytes.Repeat([]byte{0}, shareWidth-consts.NamespaceSize) out of the for-loop? The compiler is probably smart enough to optimize this anyways but it would be slightly more readable with a proper var name imo.
There was a problem hiding this comment.
Actually, I think we can move out not just repeat but the whole slice in global var to be allocated once and reused everywhere. Shares are immutable so this should work and save us some RAM 😌
Furthermore, using a pointer for NamespaceShare instead of copying value should also give us a tiny win, though still a win. But this should be tackled together with whole Share struct revisiting, as it currently has an ID field which is totally unused, but still exist
types/share_splitting.go
Outdated
| // tail is filler for all tail padded shares | ||
| // it is allocated once and used everywhere | ||
| var tail = append( |
There was a problem hiding this comment.
nit: maybe a slightly more descriptive name? it's not really necessary from a documentation standpoint, but it is a package var
There was a problem hiding this comment.
Would you mind suggesting the name, as a native English speaker?
There was a problem hiding this comment.
tailPaddingShare? just to match TailPaddingNamespace
There was a problem hiding this comment.
tailPaddingShare is good IMO. I know Hlib prefers shorter var names but in this case the (global) variable should have a descriptive name. A shorter name would be OK, if the var was in the scope of the function itself.
There was a problem hiding this comment.
This is dope. I agree with https://github.com/celestiaorg/lazyledger-core/pull/440/files#r661438921 but this is just a minor nit.
While wearing a debugging hat, confusing myself, and thinking about what went wrong and what could be done better, I found a better solution for #299. Also, removed one allocation in a hot path(
wrapper.Push).Addition: use one global tail padded share instead of generating them everytime