Use PVF code paired with executor params wherever possible #6742
Conversation
s0me0ne-unkn0wn
added
B0-silent
s0me0ne-unkn0wn
requested review from
eskimor,
ordian,
bkchr,
mrcnski and
sandreim
|
For question one: Given that the initial structure was already not just the PVF (code), but also the hash I think it is fine to extend the structure further and keep its original name - or pick a different one altogether if it is confusing for some reason. |
| @@ -523,7 +523,7 @@ async fn handle_execute_pvf( | |||
| artifact: ArtifactPathId::new(artifact_id, cache_path), | |||
| execution_timeout, | |||
| params, | |||
| executor_params: pvf_with_params.executor_params(), | |||
| executor_params: (*pvf_with_params.executor_params()).clone(), | |||
There was a problem hiding this comment.
Why is executor_params an Arc? I played around with removing it and it seems to work fine, and not having an Arc simplified a lot of these clones etc. Or am I missing something? 🙂
There was a problem hiding this comment.
Two considerations:
- I think you remember why that
Arcappeared in the first place: because we want the whole structure to be easily clonable; - There is already a method called
code()(inherited from olderPvfstruct) which returnsArc, and having two getters returning different types of references seems like an inconsistent design to me.
Probably you're right, and things shouldn't be overcomplicated here. I just need to either persuade myself it's okay or use some name like executor_params_as_ref().
There was a problem hiding this comment.
I think you remember why that Arc appeared in the first place: because we want the whole structure to be easily clonable
Oh right, I remember that now. It's even there in the docstring.
* master: (98 commits) Ensure max_weight is assigned properly in AllowTopPaidExecutionFrom (#6787) Explicitly Handling ProvisionableData Cases (#6757) Companion for Substrate#12520 (#6730) Revert back to bare metal runners for weights generation (#6762) Improve XCM fuzzer (#6190) Corrected weight trader comment (#6752) clean up executed migrations (#6763) Remove state migration from westend runtime. (#6737) polkadot companion #12608 (Pools claim permissions) (#6753) Add Turboflakes bootnodes to Polkadot, Kusama and Westend (#6628) Companion for substrate#13284 (#6653) Companion for Substrate #13410: Introduce EnsureOrigin to democracy.propose (#6750) `BlockId` removal: `BlockBuilderProvider::new_block_at` (#6734) Companion PR for PR#13119 (#6683) Companion for Substrate#13411: frame/beefy: prune entries in set id session mapping (#6743) `BlockId` removal: refactor of runtime API (#6721) Fix auction bench (#6747) Use PVF code paired with executor params wherever possible (#6742) Retire `OldV1SessionInfo` (#6744) Companion for substrate #13121 - BEEFY Equivocations support (#6593) ...
Follow-up of #6161, closes #6724
The preparation pool and queue have been reworked.
The execution queue is untouched, as the PVF code and the executor params are handled separately there: executor params are per worker, and PVF is per job in the form of an already prepared artifact, so it doesn't make sense to pass plain code there.
Some questions I still have in mind:
PvfWithExecutorParamsfor the sake of clarity or rename it back to justPvffor the sake of conciseness;ExecutorParamsby reference (I abandoned the idea of passing it as so in queues as it causes lifetimes hell).