Use im::HashMap for storage_changes

[sorpaas]

rel #9427

This uses im::HashMap for Account::storage_changes. In the future we should refactor to use PDS for other account changes, but right now I'm still figuring out some entanglement related to state and state_db mod.

The previous bottleneck was in Account::clone_dirty, where we do a full clone of all changes at each checkpoint. This PR relieves that issue. The complexity comparison is as follows:

• Get: O(1) -> O(logn)
• Insert: O(1) -> O(logn)
• Clone: O(n) -> O(1)

Note that O(logn) is actually a small number -- if a storage uses up all 256-bit variations and we consider the complexity to be O(log(2)n), then it will only take at most 256 hops.

I'm still thinking about how to do a benchmark on this change, but currently haven't thought of a good way.

[dvdplm]

Cool stuff. Exciting to see the benchmark of this!

[dvdplm]

maybe just use im::HashMap as IMHashMap;

[dvdplm]

same here

[dvdplm]

Do you plan to add benchmarks in this PR or in a separate one? The potential performance impact is my only concern at this point.

[ordian]

I agree with David, that since the whole point of switching is performance optimization, it would be nice to have some benchmarks to justify the changes. Also note, that im is a thread-safe version of the library, there is also im-rc, which is thread unsafe and faster. Although maybe we can eliminate some locks with im.

[andresilva]

Changes LGTM but I'd also like to see some benchmarks. Also as @ordian mentioned maybe we can switch to the thread unsafe version since we won't be removing any locks at this point?

[lexfrl]

Wow, looks like this is a first usage of the persistent data structures in parity-ethereum! Would be great to take a look on some benchmarks also! 👍

[lexfrl]

This is not quite related to this specific PR, but I just wonder, what will happen if it'll fail to commit? Should we concern about this?

[sorpaas]

Then that error will be propagate out and cause the block import to fail. I think we handled that correctly in Client.

[sorpaas]

I did a benchmark on this, it's actually slower:

	#[test]
	fn storage_bench() {
		use std::time::{Duration, Instant};
		let now = Instant::now();

		let a = 10.into();

		let db = get_temp_state_db();
		let mut state = State::new(db, U256::from(0), Default::default());

		for i in 0..1000000 {
			state.set_storage(&a, H256::from(&U256::from(i)), H256::from(&U256::from(20u64))).unwrap();
		}

		state.checkpoint();
		state.set_storage(&a, H256::from(&U256::from(0)), H256::from(&U256::from(20u64))).unwrap();

		for i in 0..1000000 {
			state.storage_at(&a, &H256::from(&U256::from(i))).unwrap();
		}

		println!("Elapsed: {:?}", now.elapsed());
	}

I think the reason is due to Arc, whose locks give performance penalty. I'm re-thinking about the plan on #9427:

• With EIP 1283: Net gas metering for SSTORE without dirty maps #9319, we have actually already get performance gain in some cases (in the above test, it's 1.5x speed up) because of this (https://github.com/paritytech/parity-ethereum/pull/9319/files#diff-f626e5fe7236214aee0072a7d8ee53d7R289).
• Most storage changes are actually really small, and our hardware is designed to make copying small chunks of memory really efficient.
• Checkpoint iteration (https://github.com/paritytech/parity-ethereum/pull/9319/files#diff-fff79319205229b71cd0b01cdcb4eedbR616) might not be an issue -- it has an upper bound and we're only checking for the first checkpoint where that entry exists.

So for the short-term, I think it would be okay to keep the current state checkpointing logic as is -- I don't think that's part of our current performance bottleneck right now.

[dvdplm]

Thank you for trying this out, these things can be subtle! :)

Most storage changes are actually really small,

Is it true that while most might be really small, there are some that are really big?

[sorpaas]

Is it true that while most might be really small, there are some that are really big?

Yes that's true. Some indeed can be big.