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

Subsystems memory tracking: 1. Transaction pool #4822

Merged
merged 25 commits into from
Feb 7, 2020
Merged

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Feb 4, 2020

There will be extra some bounds on runtime primitives, as proposed in #4806

If some project uses exotic types for OpaqueExtrinsic/Hash, they might need to implement MallocSizeOf for it also

@NikVolf NikVolf added A0-please_review Pull request needs code review. B1-clientnoteworthy labels Feb 4, 2020
@NikVolf NikVolf requested a review from bkchr February 4, 2020 14:39
@NikVolf NikVolf added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Feb 4, 2020
@NikVolf NikVolf added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 4, 2020
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

The txpool looks good, but I'm not sure about pulluting runtime stuff (frame specifically, primitives is probably fine).

client/cli/src/informant.rs Outdated Show resolved Hide resolved
client/transaction-pool/graph/src/future.rs Outdated Show resolved Hide resolved
client/transaction-pool/graph/src/ready.rs Outdated Show resolved Hide resolved
client/transaction-pool/graph/src/ready.rs Show resolved Hide resolved
client/transaction-pool/graph/src/ready.rs Outdated Show resolved Hide resolved

/// The output of the `Hashing` function.
type Hash:
Parameter + Member + MaybeSerializeDeserialize + Debug + MaybeDisplay + SimpleBitOps + Ord
+ Default + Copy + CheckEqual + sp_std::hash::Hash + AsRef<[u8]> + AsMut<[u8]>;
+ Default + Copy + CheckEqual + sp_std::hash::Hash + AsRef<[u8]> + AsMut<[u8]> + MaybeMallocSizeOf;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that's a pretty strong requirement to put into the runtime imho, requires bringing in a separate (parity-specific) lib to implement properly. Can't we add where clauses instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless some exotic Hash type is used, it is implemented by default within parity-util-mem (all H*, integers, fixed arrays, Vec<u8> etc. are covered)

Copy link
Contributor Author

@NikVolf NikVolf Feb 5, 2020

Choose a reason for hiding this comment

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

And we have MaybeSerializeDeserialize there, so using exotic Hash will require some work anyway.

Copy link
Contributor Author

@NikVolf NikVolf Feb 6, 2020

Choose a reason for hiding this comment

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

@bkchr what's your view on this?
@tomusdrw are you ok with the argument?

Copy link
Member

Choose a reason for hiding this comment

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

For me this is okay.

@gavofyork gavofyork changed the title Subsystems memory tacking: 1. Transaction pool Subsystems memory tracking: 1. Transaction pool Feb 5, 2020
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Overall looks good.

Some minor issues that needs to be solved :)

client/cli/src/informant.rs Show resolved Hide resolved
primitives/runtime/src/generic/unchecked_extrinsic.rs Outdated Show resolved Hide resolved
primitives/runtime/src/lib.rs Outdated Show resolved Hide resolved
primitives/runtime/src/lib.rs Outdated Show resolved Hide resolved
primitives/runtime/src/testing.rs Outdated Show resolved Hide resolved
primitives/runtime/src/testing.rs Outdated Show resolved Hide resolved
primitives/runtime/src/testing.rs Outdated Show resolved Hide resolved
primitives/runtime/src/testing.rs Outdated Show resolved Hide resolved
primitives/runtime/src/testing.rs Show resolved Hide resolved
test-utils/runtime/src/lib.rs Show resolved Hide resolved
@NikVolf NikVolf merged commit 65763cd into master Feb 7, 2020
@NikVolf NikVolf deleted the nv-mem-txpool branch February 7, 2020 10:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants