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

Fix timer panics in the wasm light client#4561

Merged
expenses merged 144 commits into
masterfrom
ashley-polkadot-wasm
Feb 10, 2020
Merged

Fix timer panics in the wasm light client#4561
expenses merged 144 commits into
masterfrom
ashley-polkadot-wasm

Conversation

@expenses

@expenses expenses commented Jan 7, 2020

Copy link
Copy Markdown
Contributor

Last change to get light clients to work 🎉! This PR relies on @tomaka 's wasm-timer crate, but only for replacements for std::time::Instant and std::time::SystemTime.

Waiting on async-rs/futures-timer#51.

tomaka and others added 30 commits November 20, 2019 16:47
Comment thread client/cli/src/runtime.rs Outdated
Comment thread client/informant/src/lib.rs Outdated
@expenses

expenses commented Feb 6, 2020

Copy link
Copy Markdown
Contributor Author

On ice as we're waiting for libp2p/rust-libp2p#1414.

@expenses expenses removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 6, 2020
@tomaka

tomaka commented Feb 6, 2020

Copy link
Copy Markdown
Contributor

Why not merge this? It's basically ready.

@amaury1093 amaury1093 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I tried the wasm node and it works well.

@expenses expenses requested a review from tomaka February 7, 2020 00:38
@expenses

expenses commented Feb 7, 2020

Copy link
Copy Markdown
Contributor Author

@tomaka it say that you've still got requested changes apparently.

Comment thread client/informant/Cargo.toml Outdated
sc-service = { version = "0.8", default-features = false, path = "../service" }
sp-blockchain = { version = "2.0.0", path = "../../primitives/blockchain" }
sp-runtime = { version = "2.0.0", path = "../../primitives/runtime" }
parity-util-mem = { version = "0.5.1", default-features = false, features = ["primitive-types"] }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: could you order alphabetically?

@expenses expenses requested a review from tomaka February 7, 2020 21:12
"disk_read_per_sec" => info.usage.as_ref().map(|usage| usage.io.bytes_read).unwrap_or(0),
"disk_write_per_sec" => info.usage.as_ref().map(|usage| usage.io.bytes_written).unwrap_or(0),
);
#[cfg(not(target_os = "unknown"))]

@tomaka tomaka Feb 10, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is that now needed?
Also, why not merge the PR when I approved it, instead of sneaking in more and more amendments that need a re-review?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because if I did that, it would have panicked when you started the light client. We now need to do it this way because you can't call parity_util_mem::malloc_size on wasm because wasm_timer::wasm::Instant doesn't impl MallocSizeOf.

@tomaka tomaka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

x

@tomaka tomaka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I must admit that I'm a bit tired of this PR and agree that we should just merge it.
The #[cfg] blocks reaaally should be removed sooner or later though.

@expenses expenses merged commit ae03ee9 into master Feb 10, 2020
@expenses expenses deleted the ashley-polkadot-wasm branch February 10, 2020 11:23
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.

7 participants