Issue 4393: Correcting Unnecessary Use of Arc #6882
Conversation
…brad-issue-4393
BradleyOlson64
added
A0-please_review
| @@ -160,6 +160,21 @@ impl PartialEq for ParticipationRequest { | |||
| } | |||
| #[cfg(test)] | |||
| impl Eq for ParticipationRequest {} | |||
| #[cfg(test)] | |||
| impl Clone for ParticipationRequest { | |||
There was a problem hiding this comment.
I would just not have implemented Clone at all. This is a bit hacky. Instead of cloning, we should be able to create new instances in tests where necessary. E.g. just a new let ... = create_test_partitipation_request ... instead of cloning the previous one.
There was a problem hiding this comment.
I went ahead and tried your suggestion. It turns out there's some non-determinism in the helper make_participation_request() such that the candidate hashes end up different. Particularly the signature field in the CandidateDescriptor within the CandidateReceipt is different each time it is generated.
This means that calling make_participation_request() multiple times results in requests that don't compare as equal, ruining our asserts.
Know of other ways we can do this without clone?
There was a problem hiding this comment.
Or here's another idea which might be considered better form.
I could at least move the clone functionality into tests.rs as a helper function rather than implementing it on the ParticipationRequest type.
There was a problem hiding this comment.
yes, making it a standalone function like test_clone core something would also be way better. The non-determinism is interesting, I don't see where it would be coming from?
| fn clone_from(&mut self, source: &Self) { | ||
| *self = source.clone(); | ||
| } |
There was a problem hiding this comment.
True. My understanding is that I need to provide this function as a part of the Clone implementation. Is there a better way to handle these unused but required parts of a type?
There was a problem hiding this comment.
Oh, I see it's a part of the Clone trait! I've never seen it before. But it doesn't seem to be a required method, so you can remove it. The doc for it is interesting:
a.clone_from(&b)is equivalent toa = b.clone()in functionality, but can be overridden to reuse the resources of a to avoid unnecessary allocations.
* master: (50 commits) Issue 4393: Correcting Unnecessary Use of Arc (#6882) Companion for #13287 (#6655) timestamp ci job logs (#6890) Release parachain host API v4 (#6885) polkadot companion: #13128 (Pools commission) (#6264) companion for #13555 (#6842) Bump libgit2-sys from 0.14.1+1.5.0 to 0.14.2+1.5.1 (#6600) Bump bumpalo from 3.8.0 to 3.12.0 (#6599) Bump git2 from 0.16.0 to 0.16.1 (#6601) Council as SpendOrigin (#6877) PVF: Document that preparation cannot lead to disputes (#6873) Sync versions with current release (0.9.39) (#6875) Companion for paritytech/substrate#13592 (#6869) Update orchestra to the recent version (#6854) Delete unused Cargo.lock (#6870) Remove use of Store trait (#6835) Companion for Substrate #13564 (#6845) Adding Dispute Participation Metrics (#6838) Update `substrate` to 48e7cb1 (#6851) Move PVF timeouts to executor environment parameters (#6823) ...
…slashing-client * ao-past-session-slashing-runtime: (23 commits) Issue 4393: Correcting Unnecessary Use of Arc (#6882) Companion for #13287 (#6655) timestamp ci job logs (#6890) Release parachain host API v4 (#6885) polkadot companion: #13128 (Pools commission) (#6264) companion for #13555 (#6842) Bump libgit2-sys from 0.14.1+1.5.0 to 0.14.2+1.5.1 (#6600) Bump bumpalo from 3.8.0 to 3.12.0 (#6599) Bump git2 from 0.16.0 to 0.16.1 (#6601) Council as SpendOrigin (#6877) PVF: Document that preparation cannot lead to disputes (#6873) Sync versions with current release (0.9.39) (#6875) Companion for paritytech/substrate#13592 (#6869) Update orchestra to the recent version (#6854) Delete unused Cargo.lock (#6870) Remove use of Store trait (#6835) Companion for Substrate #13564 (#6845) Adding Dispute Participation Metrics (#6838) Update `substrate` to 48e7cb1 (#6851) Move PVF timeouts to executor environment parameters (#6823) ...
Follow up PR to #6838
Modified
ParticipationRequestso that its fieldrequest_timerdoesn't need to use Arc.Also may change histogram metric bucket sizes with this fix.