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

Reuse wasmtime instances, the PR #5567

Merged
merged 8 commits into from
Apr 8, 2020
Merged

Reuse wasmtime instances, the PR #5567

merged 8 commits into from
Apr 8, 2020

Conversation

pepyakin
Copy link
Contributor

@pepyakin pepyakin commented Apr 7, 2020

Fixes #5141

So far, we have bug #5036. TL;DR: Basically, the linear memory is not completely zeroed. It is zeroed enough for avoiding the issues #3011 was set to fix, but still not completely.

What it means is that the contents of the memory are preserved from the previous call. I don't yet fully know if there any ramifications of this approach. There are some thoughts on this in this comment, again TL;DR I am thinking if this is OK or not.

So this PR fixes #5141 as if #5036 was not a bug.

I am not absolutely sure if I understand all ramifications treating #5036 as not a bug though and if we want to go the way I outlined in the comment. So this PR gives us a point for doing the change, because AFAICT, it yields more performance than the current implementation.

In any case, this is still a useful PR that improves the status quo.

@pepyakin pepyakin added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes labels Apr 7, 2020
@pepyakin pepyakin added this to the 2.0 milestone Apr 7, 2020
@pepyakin pepyakin changed the title Reuse wasmtime instances Reuse wasmtime instances, the PR Apr 7, 2020
@pepyakin pepyakin added B1-clientnoteworthy and removed B0-silent Changes should not be mentioned in any release notes labels Apr 8, 2020
@paritytech paritytech deleted a comment from parity-benchapp bot Apr 8, 2020
@parity-benchapp
Copy link

parity-benchapp bot commented Apr 8, 2020

Finished benchmark for branch: ser-reuse-instances

Benchmark: Import Benchmark via wasm (random transfers)

Master: 728.80 ms
Branch: 722.09 ms
Change: -0.92%

@NikVolf
Copy link
Contributor

NikVolf commented Apr 8, 2020

/bench import

@parity-benchapp
Copy link

parity-benchapp bot commented Apr 8, 2020

Finished benchmark for branch: ser-reuse-instances

Benchmark: Import Benchmark (random transfers)

Master: 696.77 ms
Branch: 694.00 ms
Change: -0.40%

@bkchr
Copy link
Member

bkchr commented Apr 8, 2020

@NikVolf this should show improvements for syncing and not really for importing single blocks.

@arkpar
Copy link
Member

arkpar commented Apr 8, 2020

Do these benches even use wasmtime? And if so, do they account for compilation overhead?

@arkpar arkpar merged commit 5d05c9c into master Apr 8, 2020
@arkpar arkpar deleted the ser-reuse-instances branch April 8, 2020 16:45
@NikVolf
Copy link
Contributor

NikVolf commented Apr 8, 2020

Do these benches even use wasmtime? And if so, do they account for compilation overhead?

they do, there is an issue with rocksdb cache for benchmarks currently though

@NikVolf
Copy link
Contributor

NikVolf commented Apr 8, 2020

@NikVolf this should show improvements for syncing and not really for importing single blocks.

@pepyakin said they should show improvement, so we decided to run them

@NikVolf
Copy link
Contributor

NikVolf commented Apr 8, 2020

And if so, do they account for compilation overhead

they do account for compilation overhead

pepyakin added a commit that referenced this pull request Apr 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reuse wasmtime instances
4 participants