Improve documentation #56

adlerjohn · 2021-04-23T14:15:32Z

Fixes #11

codecov · 2021-04-23T14:16:29Z

Codecov Report

Merging #56 (e500007) into master (f474268) will increase coverage by 2.47%.
The diff coverage is 100.00%.

@@            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

Impacted Files	Coverage Δ
datasquare.go	`92.92% <ø> (ø)`
extendeddatacrossword.go	`71.42% <100.00%> (+1.36%)`	⬆️
leopard.go	`100.00% <0.00%> (ø)`
extendeddatasquare.go	`40.00% <0.00%> (+0.46%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 870d7dd...e500007. Read the comment docs.

README.md

liamsi · 2021-04-28T17:32:22Z

datasquare.go

@@ -102,6 +105,7 @@ func (ds *dataSquare) rowSlice(x uint, y uint, length uint) [][]byte {
 }

 // Row returns the data in a row.
+// Do not modify this slice directly.
 func (ds *dataSquare) Row(x uint) [][]byte {


Wouldn't a non-pointer receiver be better in case we don't want consumers to modify the returned slice?

Suggested change

func (ds *dataSquare) Row(x uint) [][]byte {

func (ds dataSquare) Row(x uint) [][]byte {

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?

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.

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)

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.

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.

@liamsi so I'll leave refactoring this to a future PR, to keep this PR contained to just better documentation.

Yes, sounds good to me.

datasquare.go

evan-forbes

two 👍 👍 from me!

adlerjohn added 5 commits April 23, 2021 10:01

Improve docs.

a9202cc

Clean up and improve docs.

e6338ba

Format.

da785bc

Doc.

07baa2f

Add warning.

493cd33

adlerjohn self-assigned this Apr 23, 2021

adlerjohn added 3 commits April 24, 2021 10:09

Fix doc strings.

ade3c03

Clean up readme.

3a848fb

Add example to readme.

24c6827

adlerjohn marked this pull request as ready for review April 27, 2021 20:51

adlerjohn requested review from liamsi and evan-forbes April 27, 2021 20:51

liamsi reviewed Apr 28, 2021

View reviewed changes

Fix example to not require cgo.

e500007

adlerjohn mentioned this pull request Apr 29, 2021

Clarify public API #12

Closed

adlerjohn requested a review from liamsi April 29, 2021 15:23

evan-forbes approved these changes Apr 29, 2021

View reviewed changes

adlerjohn merged commit 5a692bf into master Apr 29, 2021

adlerjohn deleted the adlerjohn/improve_docs branch April 29, 2021 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve documentation #56

Improve documentation #56

adlerjohn commented Apr 23, 2021 •

edited

Loading

codecov bot commented Apr 23, 2021 •

edited

Loading

liamsi Apr 28, 2021

adlerjohn Apr 28, 2021 •

edited

Loading

liamsi Apr 28, 2021

evan-forbes Apr 29, 2021

liamsi Apr 29, 2021

adlerjohn Apr 29, 2021

adlerjohn Apr 29, 2021

liamsi Apr 29, 2021

evan-forbes left a comment

	func (ds *dataSquare) Row(x uint) [][]byte {
	func (ds dataSquare) Row(x uint) [][]byte {

Improve documentation #56

Improve documentation #56

Conversation

adlerjohn commented Apr 23, 2021 • edited Loading

codecov bot commented Apr 23, 2021 • edited Loading

Codecov Report

liamsi Apr 28, 2021

Choose a reason for hiding this comment

adlerjohn Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

liamsi Apr 28, 2021

Choose a reason for hiding this comment

evan-forbes Apr 29, 2021

Choose a reason for hiding this comment

liamsi Apr 29, 2021

Choose a reason for hiding this comment

adlerjohn Apr 29, 2021

Choose a reason for hiding this comment

adlerjohn Apr 29, 2021

Choose a reason for hiding this comment

liamsi Apr 29, 2021

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

adlerjohn commented Apr 23, 2021 •

edited

Loading

codecov bot commented Apr 23, 2021 •

edited

Loading

adlerjohn Apr 28, 2021 •

edited

Loading