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.

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

Try further optimize ReconstructSome for leopard 8+16 #274

Closed
wants to merge 4 commits into from

Conversation

liamsi
Copy link

@liamsi liamsi commented Mar 4, 2024

Trying:

This might lead to failing tests. But if not this should be much faster than before.

builds on changes introduced in #272

@liamsi liamsi marked this pull request as ready for review March 4, 2024 14:15
@liamsi liamsi marked this pull request as draft March 4, 2024 14:15
@liamsi liamsi changed the title Elias orijtech/master Try further optimize ReconstructSome for leopard 8+16 Mar 4, 2024
errorBits.set(i)
}
}
for i := 0; i < r.dataShards; i++ {
if len(shards[i]) == 0 {
if len(shards[i]) == 0 /*&& (recoverAll || (required != nil && required[i]))*/ {
Copy link
Author

Choose a reason for hiding this comment

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

Uncommenting this leads to more significant improvements in BenchmarkDecode1K but will lead to a bunch of test failures (I think mostly because ReconstructData expects to reconstruct all original missing data). This can probably be fixed.

errorBits.set(i)
}
}
for i := 0; i < r.dataShards; i++ {
if len(shards[i]) == 0 {
if len(shards[i]) == 0 /*&& (recoverAll || (required != nil && required[i]))*/ {
Copy link
Author

Choose a reason for hiding this comment

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

@klauspost
Copy link
Owner

Note that ReconstructSome isn't the most well-tested function in the library (it was mostly a contribution), so I wouldn't take tests passing as gospel that everything is all right.

I suggest adding some more detailed tests.

@liamsi
Copy link
Author

liamsi commented Mar 4, 2024

Yeah, that makes sense. I was hoping that @elias-orijtech could try to optimize, test, and benchmark this further. I think particularly the errorBits bitfield is rather rarely used in tests:

useBits := r.totalShards-numberPresent <= r.parityShards/4

I was also surprised it is constructed at all if we already know with the line if it is needed or not (I'm guessing it is rather inexpensive but it seems somewhat unnecessary.

@liamsi
Copy link
Author

liamsi commented Apr 16, 2024

Closing in favour of #272

@liamsi liamsi closed this Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants