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

use a Event Cache to replace vec Events in deposit_event#2228

Closed
atenjin wants to merge 2 commits intoparitytech:masterfrom
atenjin:master
Closed

use a Event Cache to replace vec Events in deposit_event#2228
atenjin wants to merge 2 commits intoparitytech:masterfrom
atenjin:master

Conversation

@atenjin
Copy link
Contributor

@atenjin atenjin commented Apr 8, 2019

issue 2223 #2223
suggestion 1
use a map EventsCache to replace vec Events in deposit_event, and flush cache into Events on finalize()

issue 2223 paritytech#2223
suggestion 1
use  a map `EventsCache` to replace vec `Events` in `deposit_event`, and flush cache into `Events` on `finalize()`
@parity-cla-bot
Copy link

It looks like @jkingdom signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

1 similar comment
@parity-cla-bot
Copy link

It looks like @jkingdom signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@bkchr
Copy link
Member

bkchr commented Apr 8, 2019

I still think this should be solved in parity-codec.

@atenjin
Copy link
Contributor Author

atenjin commented Apr 8, 2019

I still think this should be solved in parity-codec.

Unless provide list storage for decl_storage, it's obvious that the Vec decode/encode would cast linear growth(O(n)) with more elements, no matter how to optimize it, it can't be changed to O(1).

I think substrate should not open an O(n) storage to users, for users would access it in their way which may cast performance problem just like deposit a lot events

@xlc
Copy link
Contributor

xlc commented Apr 8, 2019

I agree with @jkingdom
The complexity of the old method is O(n^2) as the complexity of the insertion of new event is O(n/2) and it is performed n times, and resulting O(n*n/2), i.e. O(n^2)
This approach make it O(n).
I fail to see how how optimization of parity-codec can reduce complexity. Unless it is overheadless, make the complexity of insertion become O(1).
It is actually possible, because append item to end of vector does not need to deserialize the vector at all. All it need is append the bytes, and read the length prefix, and update the length prefix.
But again, I find it hard to image how parity-codec needs to be changed to support this optimization.

@bkchr
Copy link
Member

bkchr commented Apr 8, 2019

I would provide a function append or similar for some encoded types (as an extra trait). Each object that implements this new trait, supports appending of data. In the case of the Vec it would work in the following way:

  1. Decode just the length of the vector.
  2. Length + 1 and encode it.
  3. Check if new_length_encoded_len = old_length_encoded_len
    4a) If 3 is true, overwrite the length at the beginning of the blob and add the encoded new data at the end of the blob.
    4b) If 3 is false, allocate new_lenght_encoded_len + rest_blob_len + new_data_encoded_len, and write all data into this new allocated space.

So we will require just one database access for the event data. If we expect database accesses to cost more than allocating space. My solution should be fast than the current presented one.

@atenjin
Copy link
Contributor Author

atenjin commented Apr 9, 2019

I would provide a function append or similar for some encoded types (as an extra trait). Each object that implements this new trait, supports appending of data. In the case of the Vec it would work in the following way:

  1. Decode just the length of the vector.
  2. Length + 1 and encode it.
  3. Check if new_length_encoded_len = old_length_encoded_len
    4a) If 3 is true, overwrite the length at the beginning of the blob and add the encoded new data at the end of the blob.
    4b) If 3 is false, allocate new_lenght_encoded_len + rest_blob_len + new_data_encoded_len, and write all data into this new allocated space.

So we will require just one database access for the event data. If we expect database accesses to cost more than allocating space. My solution should be fast than the current presented one.

this is a nice solution, but it may modify all related interfaces, I think at least related to sr_io, storage/mod.rs, parity_code, etc...
and this solution just improve for Vec, I hope you can provide it for BTreeMap as well, thx.

It may cast a lot of time, at now, we(ChainPool, ChainX) fork substrate to do our changes for this.

@bkchr
Copy link
Member

bkchr commented Apr 9, 2019

Yeah for BTreeMap it is probably not that easy, because you don't know if you have any duplicates. The strategy would probably look a little bit different.
Good that you have a solution for the moment, I will try to work on this and inform you when it is done.

@bkchr
Copy link
Member

bkchr commented Apr 13, 2019

Just to give an update on this. I have made the changes to parity-codec I described above. In a benchmark I achieve a speedup of factor 366, by using my described method(benchmark is appending 1000 items to a vector like it would be done in substrate).

@bkchr
Copy link
Member

bkchr commented Apr 16, 2019

Superseded by: #2282

@bkchr bkchr closed this Apr 16, 2019
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.

5 participants