Conversation
Codecov Report
@@ Coverage Diff @@
## master #56 +/- ##
==========================================
+ Coverage 74.22% 76.70% +2.47%
==========================================
Files 8 9 +1
Lines 388 382 -6
==========================================
+ Hits 288 293 +5
+ Misses 62 57 -5
+ Partials 38 32 -6
Continue to review full report at Codecov.
|
|
|
||
| // Row returns the data in a row. | ||
| // Do not modify this slice directly. | ||
| func (ds *dataSquare) Row(x uint) [][]byte { |
There was a problem hiding this comment.
Wouldn't a non-pointer receiver be better in case we don't want consumers to modify the returned slice?
| func (ds *dataSquare) Row(x uint) [][]byte { | |
| func (ds dataSquare) Row(x uint) [][]byte { |
There was a problem hiding this comment.
But that would copy the entire data square each time one of these methods is called, no? Wouldn't it better to make the exported methods return a copy of the slice, and make new internal methods that return a slice?
There was a problem hiding this comment.
Fair enough. Note that the type dataSquare is unexported itself. But then it is just embedded into an ExtendedDataSquare. Hence these methods on an unexported type are still public API (which isn't obvious when looking at the extended data square). Ideally, we would make this more explicit, too.
Wouldn't it better to make the exported methods return a copy of the slice
Yeah, probably. I guess this is orthogonal for this PR and we can track it in a separate issue.
There was a problem hiding this comment.
I might have messed up running the benchmarks, but removing the pointers for read only methods doesn't seem to make a noticeable difference time wise. I like the exported copy method too. Regardless, I definitely agree that it's orthogonal.
name old time/op new time/op delta
Encoding/Encoding_128_shares_using_RSGF8-16 269µs ± 0% 259µs ± 0% ~ (p=1.000 n=1+1)
Encoding/Encoding_128_shares_using_LeopardFF8-16 145µs ± 0% 149µs ± 0% ~ (p=1.000 n=1+1)
Encoding/Encoding_128_shares_using_LeopardFF16-16 143µs ± 0% 146µs ± 0% ~ (p=1.000 n=1+1)
Decoding/Decoding_128_shares_using_RSGF8-16 57.4µs ± 0% 55.8µs ± 0% ~ (p=1.000 n=1+1)
Decoding/Decoding_128_shares_using_LeopardFF8-16 283µs ± 0% 281µs ± 0% ~ (p=1.000 n=1+1)
Decoding/Decoding_128_shares_using_LeopardFF16-16 288µs ± 0% 275µs ± 0% ~ (p=1.000 n=1+1)
Roots/Square_Size_32x32-16 1.73µs ± 0% 2.55µs ± 0% ~ (p=1.000 n=1+1)
Roots/Square_Size_64x64-16 3.68µs ± 0% 5.34µs ± 0% ~ (p=1.000 n=1+1)
Roots/Square_Size_128x128-16 6.53µs ± 0% 9.70µs ± 0% ~ (p=1.000 n=1+1)
Roots/Square_Size_256x256-16 9.49µs ± 0% 12.81µs ± 0% ~ (p=1.000 n=1+1)
Repair/Repairing_16x16_ODS_using_RSGF8-16 7.05ms ± 0% 7.25ms ± 0% ~ (p=1.000 n=1+1)
Repair/Repairing_16x16_ODS_using_LeopardFF8-16 9.83ms ± 0% 9.68ms ± 0% ~ (p=1.000 n=1+1)
Repair/Repairing_16x16_ODS_using_LeopardFF16-16 10.1ms ± 0% 9.9ms ± 0% ~ (p=1.000 n=1+1)
Repair/Repairing_32x32_ODS_using_RSGF8-16 33.8ms ± 0% 33.4ms ± 0% ~ (p=1.000 n=1+1)
Repair/Repairing_32x32_ODS_using_LeopardFF8-16 45.2ms ± 0% 43.5ms ± 0% ~ (p=1.000 n=1+1)
Repair/Repairing_32x32_ODS_using_LeopardFF16-16 44.4ms ± 0% 44.8ms ± 0% ~ (p=1.000 n=1+1)
Repair/Repairing_64x64_ODS_using_LeopardFF16-16 121ms ± 0% 118ms ± 0% ~ (p=1.000 n=1+1)
Repair/Repairing_64x64_ODS_using_RSGF8-16 104ms ± 0% 103ms ± 0% ~ (p=1.000 n=1+1)
Repair/Repairing_64x64_ODS_using_LeopardFF8-16 119ms ± 0% 122ms ± 0% ~ (p=1.000 n=1+1)
Repair/Repairing_128x128_ODS_using_RSGF8-16 437ms ± 0% 442ms ± 0% ~ (p=1.000 n=1+1)
Repair/Repairing_128x128_ODS_using_LeopardFF8-16 413ms ± 0% 423ms ± 0% ~ (p=1.000 n=1+1)
Repair/Repairing_128x128_ODS_using_LeopardFF16-16 407ms ± 0% 411ms ± 0% ~ (p=1.000 n=1+1)
Extension/RSGF8_size_4-16 36.6µs ± 0% 37.6µs ± 0% ~ (p=1.000 n=1+1)
Extension/LeopardFF8_size_4-16 84.9µs ± 0% 89.7µs ± 0% ~ (p=1.000 n=1+1)
Extension/LeopardFF16_size_4-16 85.5µs ± 0% 87.9µs ± 0% ~ (p=1.000 n=1+1)
Extension/RSGF8_size_8-16 165µs ± 0% 159µs ± 0% ~ (p=1.000 n=1+1)
Extension/LeopardFF8_size_8-16 326µs ± 0% 334µs ± 0% ~ (p=1.000 n=1+1)
Extension/LeopardFF16_size_8-16 315µs ± 0% 334µs ± 0% ~ (p=1.000 n=1+1)
Extension/RSGF8_size_16-16 902µs ± 0% 915µs ± 0% ~ (p=1.000 n=1+1)
Extension/LeopardFF8_size_16-16 1.34ms ± 0% 1.39ms ± 0% ~ (p=1.000 n=1+1)
Extension/LeopardFF16_size_16-16 1.33ms ± 0% 1.29ms ± 0% ~ (p=1.000 n=1+1)
Extension/RSGF8_size_32-16 5.71ms ± 0% 5.80ms ± 0% ~ (p=1.000 n=1+1)
Extension/LeopardFF8_size_32-16 6.43ms ± 0% 6.06ms ± 0% ~ (p=1.000 n=1+1)
Extension/LeopardFF16_size_32-16 6.25ms ± 0% 6.12ms ± 0% ~ (p=1.000 n=1+1)
Extension/RSGF8_size_64-16 23.7ms ± 0% 22.9ms ± 0% ~ (p=1.000 n=1+1)
Extension/LeopardFF8_size_64-16 16.5ms ± 0% 16.6ms ± 0% ~ (p=1.000 n=1+1)
Extension/LeopardFF16_size_64-16 16.8ms ± 0% 17.1ms ± 0% ~ (p=1.000 n=1+1)
Extension/RSGF8_size_128-16 133ms ± 0% 130ms ± 0% ~ (p=1.000 n=1+1)
Extension/LeopardFF8_size_128-16 48.7ms ± 0% 49.0ms ± 0% ~ (p=1.000 n=1+1)
Extension/LeopardFF16_size_128-16 50.1ms ± 0% 50.6ms ± 0% ~ (p=1.000 n=1+1)
There was a problem hiding this comment.
Another consideration here is consistency:
Next is consistency. If some of the methods of the type must have pointer receivers, the rest should too, so the method set is consistent regardless of how the type is used.
https://golang.org/doc/faq#methods_on_values_or_pointers
Here is what I think: make all exported methods on unexported types unexported too. Think what we want to expose and expose this more explicitly. During that refactor the question of pointer vs values will come up naturally anyways.
There was a problem hiding this comment.
but removing the pointers for read only methods doesn't seem to make a noticeable difference time wise
Ah, I think that's because it only does a shallow copy, and the majority of the data square is the slices of the shares.
There was a problem hiding this comment.
@liamsi so I'll leave refactoring this to a future PR, to keep this PR contained to just better documentation.
Fixes #11