RFC-145: Remove the host-side runtime memory allocator #145
Conversation
s0me0ne-unkn0wn
changed the title
|
This one is finally ready for review.
|
anaelleltd
added
Reviewed
|
Thanks. Didn't get around to review, yet. The priorities shifted a bit. But eventually we want that. Quick question: Why does paritytech/polkadot-sdk#8866 merge into paritytech/polkadot-sdk#8641. Can't they be merged individually to |
|
@athei Probably they can, it's just much more convenient that way. E.g., we're introducing a new version of a host function and attribute it with |
|
Ahh I see you still intent to merge paritytech/polkadot-sdk#8641 in first. Then all good 👍 |
|
|
||
| ##### Results | ||
|
|
||
| The result is the length of the output stored in the buffer provided in `out`. If the buffer is not large enough to accommodate the data, the latter will be truncated, but the full length of the output data will always be returned. |
There was a problem hiding this comment.
Is it actually useful to write partial data? I'd imagine you'll never actually make any use of the partially written output and just immediately retry with a bigger buffer, no? In which case writing out partial data seems like a waste of CPU cycles.
(This comment also applies to other host functions.)
There was a problem hiding this comment.
Fair enough 🤷♂️
Maybe not in every case, but mostly.
There was a problem hiding this comment.
I'm reopening this discussion because of problems found. Indeed, sometimes we consciously want partial reads from storage, as in .decode_len(), where only CompactU32 is read at the beginning of the value.
Allowing partial reads only in storage::read would make interfaces inconsistent, and allowing it everywhere introduces an overhead. Does it make sense to introduce an argument to storage::read that allows for partial reads? Or an entirely new function specifically for storage partial reads?
There was a problem hiding this comment.
I'm proceeding with a boolean allow_partial argument for storage-reading functions (2ec2eaf), lmk if there are any better ideas.
athei
left a comment
athei
left a comment
There was a problem hiding this comment.
Sorry for the long delay. I have finally time to review this now.
|
Finally went through the whole RFC. Looks good. Just found a few nits. I also recommend:
|
Co-authored-by: Alexander Theißen <[email protected]>
athei
left a comment
athei
left a comment
There was a problem hiding this comment.
Looks good. Just some nit around the wording of the meta field.
Co-authored-by: Alexander Theißen <[email protected]>
|
|
||
| ##### Arguments | ||
|
|
||
| * `out` is a pointer-size ([Definition 216](https://spec.polkadot.network/chap-host-api#defn-runtime-pointer-size)) to a buffer where the SCALE-encoded storage root, calculated after committing all the existing operations, will be stored. The value is actually stored only if the buffer is large enough. Otherwise, the buffer is not written into and its contents are unchanged. |
There was a problem hiding this comment.
Could we hard-code this to 32 byte instead of returning the length? I dont see much use to return it.
There was a problem hiding this comment.
I believe the point of having it SCALE-encoded is that the storage root type is generic. I'll check it once more just in case
There was a problem hiding this comment.
Okay, so in the current reference implementation storage backend is generic over the hasher type used to produce hashes, so we don't know what the root hash length is. On the other hand, we're specifying exactly Polkadot here and not some other network, and it already has established practice in that sense. I would leave it as is for now, but I'm leaving this discussion thread open to collect opinions.
| ##### Arguments | ||
|
|
||
| * `key_in` is a pointer-size ([Definition 216](https://spec.polkadot.network/chap-host-api#defn-runtime-pointer-size)) to a buffer containing a storage key; | ||
| * `key_out` is a pointer-size ([Definition 216](https://spec.polkadot.network/chap-host-api#defn-runtime-pointer-size)) to an output buffer where the next key in the storage in the lexicographical order will be written. The value is actually stored only if the buffer is large enough. Otherwise, the buffer is not written into and its contents are unchanged. |
There was a problem hiding this comment.
The runtime will be generally a bit more careful then what keys it puts into storage, or? We dont want it to suddenly do 2x as many calls just because its default key_out is too small.
There was a problem hiding this comment.
I think it's a matter of providing a reasonable buffer by default. If a runtime author wants giant keys, let him provide giant defaults ;)
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
koute
left a comment
koute
left a comment
There was a problem hiding this comment.
Looks much better! Nice job!
This gets a tentative 👍 from me assuming last remaining comments are addressed.
bkchr
left a comment
bkchr
left a comment
There was a problem hiding this comment.
Generally looking good! Ty for the work!
I left some nitpicks. Sometimes we are using i64 as return value and some times i32 when we don't return a size and just communicate the result. AFAIR WASM abi is different for both of them. So, we should maybe just always use i32 when we only communicate a result?
7 participants
Rendered