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

reed solomon perf improvements proposal #2

Merged
merged 5 commits into from
Jul 25, 2023

Conversation

drahnr
Copy link
Contributor

@drahnr drahnr commented Jul 19, 2023

@burdges
Copy link

burdges commented Jul 19, 2023

LGTM

Copy link
Contributor

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Hey Bernhard :)

When exploring SIMD, would be great to do runtime feature detection, like in dalek-cryptography/curve25519-dalek#523

applications/reed-solomon-perf.md Outdated Show resolved Hide resolved
applications/reed-solomon-perf.md Outdated Show resolved Hide resolved
applications/reed-solomon-perf.md Outdated Show resolved Hide resolved
@ordian
Copy link
Contributor

ordian commented Jul 21, 2023

Also relevant: paritytech/polkadot-sdk#598

@burdges
Copy link

burdges commented Jul 21, 2023

Also relevant: paritytech/polkadot-sdk#598

This is internal to how polkadot uses the chunks. I don't care who does this work of course. And the migration is a simply an if check to start the shuffling at some fixed block height, plus something to deprecate hosts who won't have this check. I'll send some emails about my generalized Feistel algorithm question for that.

@ordian
Copy link
Contributor

ordian commented Jul 21, 2023

I meant making the encoding systematic, which is currently not on the https://github.com/paritytech/reed-solomon-novelpoly/ level IIUC. @drahnr please correct me if I'm wrong

@burdges
Copy link

burdges commented Jul 21, 2023

I'd thought it was systematic from the beginning, but if not then yeah we should recall why we made that choice, but that'd be larger than this proposal.

Copy link

@riusricardo riusricardo left a comment

Choose a reason for hiding this comment

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

LGTM! If what @ordian is proposing would require a bigger scope, I'm okay with keeping it this way for this first part.

Co-authored-by: ordian <write@reusable.software>
@drahnr
Copy link
Contributor Author

drahnr commented Jul 21, 2023

Hey Bernhard :)

When exploring SIMD, would be great to do runtime feature detection, like in dalek-cryptography/curve25519-dalek#523

Will do, given it doesn't explode time wise

@drahnr
Copy link
Contributor Author

drahnr commented Jul 21, 2023

I'd thought it was systematic from the beginning, but if not then yeah we should recall why we made that choice, but that'd be larger than this proposal.

From the paper, it should be, I didn't verify the impl yet

Copy link
Contributor

@Noc2 Noc2 left a comment

Choose a reason for hiding this comment

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

Thanks for the application. I'm generally happy to go ahead with it. Just a few quick comments:

  • Feel free to remove the template text that isn't bold from the application, like "We expect the teams to already have a solid idea about your project's expected final state. Therefore, we ask the teams to submit (where relevant)". This makes it hard to read.
  • Could you add the programming language to the deliverables?
  • Also feel free to add

When exploring SIMD, would be great to do runtime feature detection, like in dalek-cryptography/curve25519-dalek#523

@drahnr
Copy link
Contributor Author

drahnr commented Jul 24, 2023

@Noc2 addressed your points, I am not adding any specifics on which path will be taken before looking into it and assessing the (additional) work required.

@drahnr drahnr requested review from Noc2 and ordian July 24, 2023 13:15
Copy link
Contributor

@Noc2 Noc2 left a comment

Choose a reason for hiding this comment

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

Thanks for the update. I'm happy to go ahead with it.

@riusricardo riusricardo merged commit 896060b into pioneersprize:main Jul 25, 2023
@riusricardo
Copy link

@drahnr Thanks for the application. After merging, I realized that there's a discrepancy. Could you please clarify the difference in the total amount and the amounts per milestone? The sum doesn't add up.

@drahnr
Copy link
Contributor Author

drahnr commented Jul 25, 2023

See PR paritytech/polkadot#3 which addresses the inconsistencies

@drahnr
Copy link
Contributor Author

drahnr commented Aug 7, 2023

Hey Bernhard :)
When exploring SIMD, would be great to do runtime feature detection, like in dalek-cryptography/curve25519-dalek#523

Will do, given it doesn't explode time wise

After a first time assessment, it will not be part of the milestone 1 implementation.

@drahnr
Copy link
Contributor Author

drahnr commented Feb 20, 2024

A bit of brief update, I am currently pending some clarification on the gao mateer paper by the authors. Otherwise it's well on track to delivery, albeit a bit behind schedule for a bag of personal reasons (good ones only!).

@ordian
Copy link
Contributor

ordian commented Feb 20, 2024

jFYI: paritytech/reed-solomon-novelpoly#40, SIMD impl has a 10x speedup over non-SIMD (also in the original C++ leopard lib), so it's worth looking into more closely.

@drahnr
Copy link
Contributor Author

drahnr commented Feb 21, 2024

jFYI: paritytech/reed-solomon-novelpoly#40, SIMD impl has a 10x speedup over non-SIMD (also in the original C++ leopard lib), so it's worth looking into more closely.

Jeff already mentioned it. I'd see that as part of #10 , going to make it explicit there, I don't have capacity to take on unpaid additional work beyond what I do already as open source work unfortunately.

@drahnr
Copy link
Contributor Author

drahnr commented Apr 23, 2024

So turns out I underestimated the time required for it. It's still going to be a bit before there are results. Just to be clear, further proposals will come in at a more realistic rate taking into account the uncertainty bits more.

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.

None yet

6 participants