{"action":"created","issue":{"url":"https://api.github.com/repos/paritytech/substrate/issues/885","repository_url":"https://api.github.com/repos/paritytech/substrate","labels_url":"https://api.github.com/repos/paritytech/substrate/issues/885/labels{/name}","comments_url":"https://api.github.com/repos/paritytech/substrate/issues/885/comments","events_url":"https://api.github.com/repos/paritytech/substrate/issues/885/events","html_url":"https://github.com/paritytech/substrate/pull/885","id":367481352,"node_id":"MDExOlB1bGxSZXF1ZXN0MjIwOTAyMDc5","number":885,"title":"srml-contract: refactor account db","user":{"login":"andresilva","id":123550,"node_id":"MDQ6VXNlcjEyMzU1MA==","avatar_url":"https://avatars0.githubusercontent.com/u/123550?v=4","gravatar_id":"","url":"https://api.github.com/users/andresilva","html_url":"https://github.com/andresilva","followers_url":"https://api.github.com/users/andresilva/followers","following_url":"https://api.github.com/users/andresilva/following{/other_user}","gists_url":"https://api.github.com/users/andresilva/gists{/gist_id}","starred_url":"https://api.github.com/users/andresilva/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andresilva/subscriptions","organizations_url":"https://api.github.com/users/andresilva/orgs","repos_url":"https://api.github.com/users/andresilva/repos","events_url":"https://api.github.com/users/andresilva/events{/privacy}","received_events_url":"https://api.github.com/users/andresilva/received_events","type":"User","site_admin":false},"labels":[{"id":746560613,"node_id":"MDU6TGFiZWw3NDY1NjA2MTM=","url":"https://api.github.com/repos/paritytech/substrate/labels/A4-gotissues","name":"A4-gotissues","color":"f9d0c4","default":false},{"id":746560668,"node_id":"MDU6TGFiZWw3NDY1NjA2Njg=","url":"https://api.github.com/repos/paritytech/substrate/labels/M4-core","name":"M4-core","color":"F5FCE6","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":6,"created_at":"2018-10-06T18:06:40Z","updated_at":"2018-10-16T13:32:19Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/paritytech/substrate/pulls/885","html_url":"https://github.com/paritytech/substrate/pull/885","diff_url":"https://github.com/paritytech/substrate/pull/885.diff","patch_url":"https://github.com/paritytech/substrate/pull/885.patch"},"body":"This PR rewrites the account db with support for explicit checkpointing. This guarantees that gets from the account db are much faster since they only have to lookup on a local storage map or fallback to the disk db, instead of having to recurse through a cascade of maps dependent on the call depth.\r\n\r\nI made some simplistic benchmarks to have a rough comparison of the performance changes.\r\n\r\n```\r\n name                                                 bench-base ns/iter  bench-new2 ns/iter  diff ns/iter   diff %   speedup\r\n tests::benchmarks::bench_accountdb_changeset         345,323             366,950                   21,627    6.26%    x 0.94\r\n tests::benchmarks::bench_accountdb_get               9,868               43                        -9,825  -99.56%  x 229.49\r\n tests::benchmarks::bench_accountdb_revert_noop       18,967              33,474                    14,507   76.49%    x 0.57\r\n tests::benchmarks::bench_accountdb_revert_with_data  295,011             381,688                   86,677   29.38%    x 0.77\r\n tests::benchmarks::bench_accountdb_set               80                  167                           87  108.75%    x 0.48\r\n tests::benchmarks::bench_accountdb_snapshot          18,996              10,520                    -8,476  -44.62%    x 1.81\r\n```\r\n\r\nThe `get` benchmark creates an account db with 1000 checkpoints and then performs a `get`. As expected this is much faster since the previous implementation had to recurse through 1000 maps to finish the operation. Everything else is slightly slower as expected since there's some more book-keeping and copying to do. The benchmarks also don't measure the impact of the DB cache.\r\n\r\nI still need to write a contract test that performs reverts that actually revert state changes, currently we have a test that reverts due to oog but no actual state changes were performed. Still I think this is ready for a review.\r\n\r\nFixes #480."},"comment":{"url":"https://api.github.com/repos/paritytech/substrate/issues/comments/430238625","html_url":"https://github.com/paritytech/substrate/pull/885#issuecomment-430238625","issue_url":"https://api.github.com/repos/paritytech/substrate/issues/885","id":430238625,"node_id":"MDEyOklzc3VlQ29tbWVudDQzMDIzODYyNQ==","user":{"login":"andresilva","id":123550,"node_id":"MDQ6VXNlcjEyMzU1MA==","avatar_url":"https://avatars0.githubusercontent.com/u/123550?v=4","gravatar_id":"","url":"https://api.github.com/users/andresilva","html_url":"https://github.com/andresilva","followers_url":"https://api.github.com/users/andresilva/followers","following_url":"https://api.github.com/users/andresilva/following{/other_user}","gists_url":"https://api.github.com/users/andresilva/gists{/gist_id}","starred_url":"https://api.github.com/users/andresilva/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andresilva/subscriptions","organizations_url":"https://api.github.com/users/andresilva/orgs","repos_url":"https://api.github.com/users/andresilva/repos","events_url":"https://api.github.com/users/andresilva/events{/privacy}","received_events_url":"https://api.github.com/users/andresilva/received_events","type":"User","site_admin":false},"created_at":"2018-10-16T13:32:19Z","updated_at":"2018-10-16T13:32:19Z","author_association":"MEMBER","body":"I added two new benchmarks, these benchmarks are configurable with a `Vec<usize>` that defines the number of accounts to touch at each level.\r\n\r\n* `bench_accountdb_low_depth_inserts_gets` - Sets the balance of N accounts at each level. When the maximum depth is reached we get the balance of all touched accounts. The benchmark measures the total time to insert and get the data.\r\n* `bench_accountdb_low_depth_inserts_changeset` - Sets the balance of N accounts at each level and commits all the changes. On the new implementation this means that we commit intermediate checkpoints and do a final commit to the direct database. On the old implementation this means merging the values from all intermediate calls DBs and then committing. The benchmarks measures the total time to insert and then commit the data.\r\n\r\nI ran the benchmarks with different depths and number of accounts per depth:\r\n\r\n# 100, 10, 10\r\n\r\n100 accounts at depth 1, \r\n10 accounts at depth 2, \r\n10 accounts at depth 3\r\n\r\n```\r\nname                                         old ns/iter  new ns/iter  diff ns/iter  diff %  speedup\r\nbench_accountdb_low_depth_inserts_changeset  103,977      112,956             8,979   8.64%   x 0.92\r\nbench_accountdb_low_depth_inserts_gets       10,174       17,194              7,020  69.00%   x 0.59\r\n```\r\n\r\n# 100, 30, 20, 10, 10\r\n\r\n```\r\nname                                         old ns/iter  new ns/iter  diff ns/iter  diff %  speedup\r\nbench_accountdb_low_depth_inserts_changeset  168,980      179,885            10,905   6.45%   x 0.94\r\nbench_accountdb_low_depth_inserts_gets       14,653       26,239             11,586  79.07%   x 0.56\r\n```\r\n\r\n# 10240, 512, 256, 128, 64\r\n\r\n```\r\nname                                         old ns/iter  new ns/iter  diff ns/iter  diff %  speedup\r\nbench_accountdb_low_depth_inserts_changeset  2,089,669    2,325,451         235,782  11.28%   x 0.90\r\nbench_accountdb_low_depth_inserts_gets       268,282      449,113           180,831  67.40%   x 0.60\r\n```\r\n\r\n# 10, 10, 10, 10, 10, 10, 10, 10\r\n\r\n```\r\nname                                         old ns/iter  new ns/iter  diff ns/iter  diff %  speedup\r\nbench_accountdb_low_depth_inserts_changeset  75,121       79,369              4,248   5.65%   x 0.95\r\nbench_accountdb_low_depth_inserts_gets       6,872        9,960               3,088  44.94%   x 0.69\r\n```\r\n\r\nSo overall insert performance is degraded, still when taking into account the final commit for those changes the higher cost of inserts is amortized since the current implementation will need to traverse through all the intermediary account dbs and merge all changes."}}
{"actor":{"display_login":"andresilva"}}