Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Update to upstream reed-solomon-erasure #978

Merged
merged 1 commit into from
Apr 8, 2020
Merged

Conversation

gnunicorn
Copy link
Contributor

In an attempt to investigate whether we are effected by the memory leak known in reed-solomon-erasure, this updates the dependency – switching from our fork to the upstream version, which is just ahead of ours (includes all our PR).

Unfortunately I do not know enough about this particular part of the code base to understand whether this might have unintended side-effects. Please think about that hard when reviewing.

@gnunicorn gnunicorn requested a review from rphmeier April 7, 2020 09:50
@burdges
Copy link
Contributor

burdges commented Apr 7, 2020

I never noticed this runs inside the runtime.

@rphmeier
Copy link
Contributor

rphmeier commented Apr 8, 2020

@burdges I don't believe it does - the polkadot-erasure-coding crate is not linked to the runtime in my knowledge.

@rphmeier
Copy link
Contributor

rphmeier commented Apr 8, 2020

We should not be affected by this memory leak regardless, as that code path isn't hit without parachains and we don't use a decoder more than once before discarding.

@gnunicorn gnunicorn changed the title Update to upsteram reed-solomon-erasure Update to upstream reed-solomon-erasure Apr 8, 2020
@gnunicorn gnunicorn merged commit 5f40664 into master Apr 8, 2020
@gnunicorn gnunicorn deleted the ben-update-reed-solomon branch April 8, 2020 09:30
@burdges
Copy link
Contributor

burdges commented Apr 8, 2020

We'd improve performance by using the decoder more than once however, so.. Is the erasure coding an appreciable performance hit? If so, I'll take a look at whether we want systemic coding, etc. and the available optimizations.

@rphmeier
Copy link
Contributor

rphmeier commented Apr 8, 2020

Yup, although this isn't something we've encountered yet. I'm expecting erasure-coding to be a major bottleneck.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants