Use local parity-dapps-glue instead of crate published at crates.io

[sorpaas]

No description provided.

[5chdn]

Does this only affect master?

[sorpaas]

@5chdn We still need to wait for the CI to complete to know whether this is the cause of the problem. (And this change fixes a dependency issue regardless whether it's the cause of the CI failures.)

If so, then I think it's due to cargo/rust update, specifically, a problem for querying doctest dep versions. It probably affects every build using Rust 1.27. (But there's still a chance that this may not be what caused the CI failures and I might be wrong here.)

[ordian]

I'm not sure whether this change can fix build issue, since all the commits to js-glue (#8666, #8791, #7355) after 1.9.1 was published, should not have affected default build.

[ordian]

Now that I think about it, I believe it actually fixes the build w/o cargo clean, because IIUC cargo caches git dependencies in $HOME/.cargo/registry which cargo clean doesn't remove (rust-lang/cargo#3289). And switching to path dependency invalidates this cache.

[sorpaas]

Ah right. Then I guess this needs to wait for cache cleanup right now. I'm going to reset to HEAD^.

[sorpaas]

I figured out this might not be the cache issue. I cleared up ~/.cargo and run ./test.sh locally, but it still fails with the panic strategy error.

[sorpaas]

Are there any more benefits to use panic=abort rather than binary size? I found out that it is introduced in #3423, and we had around 2mb binary gains, which, given our current binary size, doesn't look significant any more. It looks like we're one of the main project that suffers from the panic strategy error (searching google about this shows mostly parity repo on the front page). Would we consider to drop panic=abort?

I also noticed that lto optimization is disabled for profile.release, and it's introduced in #499. I didn't find explanations for this, however. Just curious, are there any particular reasons we are having this settings?

cc @arkpar

[sorpaas]

Local testing worked for me. I'm going to push the commit that disables panic=abort and enable lto optimization, just to see how it works. Looks like this also has positive effect in incremental build.

[ordian]

This is really strange, what command is failing? Is it cargo test --features "json-tests" --all --release? Because it works for me locally.

[sorpaas]

@ordian I ran ./test.sh. Without commit 8023e35, it emits the same "... the panic strategy abort which is incompatible with this crate's strategy of unwind ..." error message. Removing panic=abort fixes the issue.

My opinion on this is that panic=abort is probably not mature enough on Rust yet. While aborting saves some binary size and compile time, unwinding allows possibly more control for how the program panics (ref https://users.rust-lang.org/t/panic-unwind-vs-abort/9928). There're also other projects, like ripgrep (BurntSushi/ripgrep@d5c0454) took the same reversion.

[ordian]

The problem is ./test.sh works fine for me, so I'm not sure what the right solution is.

[ordian]

I'm afraid with lto enabled build machines can ran out of memory.

[debris]

according to the documentation lto = false is the default for [profile.release], so this changes nothing

[arkpar]

Too late for the discussion, but panic="abort" does make code execution somewhat faster. Block import was faster by about 5% iirc. I'd recommend putting it back or doing some benchmarks at least.

[sorpaas]

@arkpar I think unwinding shouldn't impact runtime speed in any significant way, because it only affects the code generated when a panic actually happened (which won't do in normal execution). Can you be more specific on how you tested it's 5% faster?

[arkpar]

Import 1m blocks from a file.
I'm not sure if rust stack unwinding is completely free for non-throwing code. Usually stack unwinding requires some additional steps in function prologue to setup the frame information. Reduced binary size is also a factor as it increases cache hit rate.

[arkpar]

I'd benchmark compilation times as well

[sorpaas]

Just FYI, to revert to panic=abort we also need to revert this PR #8999.

I don't have really strong opinion on this, but it will mean that all Drop handlers will not be called again, so database cannot shutdown clean. This may be the cause of those db corruption issue reports we received, but I can't say I'm totally sure on that.

[arkpar]

Database has to be resistant to power loss anyway.

[sorpaas]

Reading this comment (rust-lang/rfcs#1765 (comment)), for runtime performance, the difference would only be the landing pads (but it can still result in reasonable performance gain).

[sorpaas]

Reading this story (https://www.reddit.com/r/rust/comments/55ns2m/safe_and_efficient_bidirectional_trees/) it looks like landing pads are inserted together with unwrap and panic!. So with less of them we get less landing pads.

No matter whether we choose to use panic=abort or panic=unwind, it may be a good optimization to try to reduce the amount of panics we have in code, and try to move them to be checked by type system instead.