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

Lazy reaping #4895

Merged
merged 24 commits into from
Feb 24, 2020
Merged

Lazy reaping #4895

merged 24 commits into from
Feb 24, 2020

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented Feb 11, 2020

This mostly removes the functionality of OnReapAccount and introduces reference-counting on accounts to ensure that data structures referring to an account whose economic defence is the account's ED must be cleaned up before the ED is liquidated.

It's a followup/alternative to #4820

@gavofyork gavofyork added the A0-please_review Pull request needs code review. label Feb 11, 2020
@gavofyork gavofyork added the U1-asap No need to stop dead in your tracks, however issue should be addressed as soon as possible. label Feb 12, 2020
@shawntabrizi shawntabrizi self-requested a review February 18, 2020 10:46
let existed = Account::<T>::contains_key(k);
Account::<T>::insert(k, (T::Index::default(), t));
Account::<T>::insert(k, AccountInfo{ nonce: Default::default(), refcount: 0, data });
Copy link
Member

@shawntabrizi shawntabrizi Feb 18, 2020

Choose a reason for hiding this comment

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

Why is refcount/nonce returning to zero here?

The goal is just to insert some account data onto the user?

Copy link
Member Author

Choose a reason for hiding this comment

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

this looks indeed bad. good catch 👍

Copy link
Member

@shawntabrizi shawntabrizi Feb 19, 2020

Choose a reason for hiding this comment

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

Suggested change
Account::<T>::insert(k, AccountInfo{ nonce: Default::default(), refcount: 0, data });
Account::<T>::mutate(k, |a| a.data = data);

This is what you want, right?

Copy link
Member

Choose a reason for hiding this comment

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

I am going to commit this suggestion.

@paritytech paritytech deleted a comment from shawntabrizi Feb 18, 2020
gavofyork and others added 5 commits February 18, 2020 15:13
Co-Authored-By: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-Authored-By: Shawn Tabrizi <shawntabrizi@gmail.com>
@NikVolf
Copy link
Contributor

NikVolf commented Feb 21, 2020

/bench

Some final spam :)

@parity-benchapp
Copy link

parity-benchapp bot commented Feb 21, 2020

Finished benchmark for branch: gav-lazy-reaping

MASTER RESULT

import block/Wasm time:
[57.708 ms 57.826 ms 57.954 ms]

import block/Native time:
[21.069 ms 21.155 ms 21.241 ms]

BRANCH RESULT

import block/Wasm time:
[59.005 ms 59.070 ms 59.135 ms]
change:
[+1.7205% +2.0417% +2.3595%] (p = 0.00 < 0.05)
Performance has regressed.

import block/Native time:
[21.010 ms 21.113 ms 21.214 ms]
change:
[-0.8625% +0.0581% +1.0399%] (p = 0.90 > 0.05)
No change in performance detected.

@gavofyork
Copy link
Member Author

concerning that the benchmark is consistently reporting that this branch is slower.

what is it actually benchmarking? this should be waaay faster (and i believe @shawn already showed that it was).

@NikVolf
Copy link
Contributor

NikVolf commented Feb 22, 2020

concerning that the benchmark is consistently reporting that this branch is slower.

what is it actually benchmarking? this should be waaay faster (and i believe @shawn already showed that it was).

Well, it is just 2-3% (it is inside benchmark variance, so don't pay much attention, until it shows > 5-6% difference)

This benchmark is just doing import of block with 100 random transfers, and I believe code path of this pr is not touched, I mostly tested the bot on pr more or less related to performance.

I will be happy to add benchmark that should show improvements on this, btw.

@paritytech paritytech deleted a comment from parity-benchapp bot Feb 24, 2020
@paritytech paritytech deleted a comment from parity-benchapp bot Feb 24, 2020
@gnunicorn gnunicorn merged commit ddfa359 into master Feb 24, 2020
@gnunicorn gnunicorn deleted the gav-lazy-reaping branch February 24, 2020 17:04
General-Beck pushed a commit to General-Beck/substrate that referenced this pull request Mar 4, 2020
* Squash and rebase from gav-lazy-reaping

* Bump version

* Bump runtime again

* Docs.

* Remove old functions

* Update frame/balances/src/lib.rs

Co-Authored-By: Shawn Tabrizi <shawntabrizi@gmail.com>

* Update frame/contracts/src/lib.rs

Co-Authored-By: Shawn Tabrizi <shawntabrizi@gmail.com>

* Warnings

* Bump runtime version

* Update frame/democracy/src/lib.rs

Co-Authored-By: Shawn Tabrizi <shawntabrizi@gmail.com>

* Update frame/system/src/lib.rs

* Clean up OnReapAccount

* Use frame_support debug

* Bump spec

* Renames and fix

* Fix

* Fix rename

* Fix

* Increase time for test

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Benjamin Kampmann <ben.kampmann@googlemail.com>
@xlc xlc mentioned this pull request May 29, 2020
1 task
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
U1-asap No need to stop dead in your tracks, however issue should be addressed as soon as possible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants