-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support allocator inside of runtime #61
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general not sure we need this with the existence of: #4
```wat | ||
(export "alloc" (func $alloc)) | ||
(export "dealloc" (func $dealloc)) | ||
(export "realloc" (func $realloc)) | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need these with: #4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need these with: #4
It's used by entrypoint call(e.g. runtime api), right? But optional for host functions
I mentioned it in there already, it's basically orthogonal |
I don't see how #4 is orthogonal. To me, it's one or the other, as they each render the other PR pointless. The only exception is we could do #61 plus some parts of #4 (in other words not all of #4). From a pure logical point of view, I think that #4 is fundamentally a better idea because it doesn't complexify the flow of execution. When it comes to performances, I have no idea which one of the two is better and I don't think it's possible to determine that without implementing them and comparing. In my opinion, if either #61 or #4 (or #61 plus some parts of #4) is significantly faster than the other, then we should go for it, but if they compare more or less equal then we should go for #4. |
We define the spec as version 1, so the following `dummy` function `v1` MUST be exported to hint | ||
client that runtime is using version 1 spec, otherwise rollback to `legacy` allocator. | ||
The function should never be used, and its name is only for version checking. | ||
|
||
```wat | ||
(export "v1" (func $v1)) | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we need to add this export when we could just detect whether alloc
, realloc
and dealloc
are exported..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is a personal preference, if there is a v2 version, adding more features, then I always just check the version number to make sure that the subsequent logic is for that version number. Personally, I always hope that some specifications can be strongly bound to the version number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like in this RFC, I named the original allocator legac
, you can also call it v0
allocator. For other substrate implementations (smoldot or other language based substrate/polkadot), they can consider no longer supporting the old version, which is always clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than listing RFC numbers in code comments, I prefer to use version numbers to indicate new features (I hate EIPs like that)
I do not think so, it will never complexify the flow of execution, see the draft PR, it's very clear.
runtime_apis also wil encounter this problem.
This is a matter of code practice, why don't you worry about runtime_api encountering such problems?
The reason why I think it is orthogonal is that
If you care about performance, you have to compare before deciding whether to change. The goals of this RFC are completely different from #4. I don't mind #4 moving forward. This RFC just provides more possibilities for #4 BTW. |
What I am most concerned about with this PR is the versatility of the runtime, which helps the substrate runtime expand. Haven't you discovered that by combining the |
Hi @radkomih I think you will be concerned about this RFC |
Around this and #4 .. We'd ideally have the approximate goal that host calls should almost never pass owned data, only borrowed data like In principle, this requires all current host calls be replaced, not unlike #4 does, but realistically some hostcalls would still pass owned data if they really needed the owned data. We'd often permit We'd gradually gain more from this RFC as we replaced more host calls by borrowing flavors. Rust handles borrowed data nicest of course. Yet, other langauges could avoid inefficencies like call wrappers that take owned being the runtime allocator, copy it to owned data behind the system allocator, make the host call, and then allocate & copy in reverse. Instead, they'd make the hostcall directly on borrows of data managed by the runtime, either owned or borrowed. In other words, I'd think this and changes like #4 should benefit each other. |
@bkchr Hi, want to know your thinking. |
Just my two cents: I'm in favor of #4 over this RFC, and I also strongly dislike the the fact that this would require us to call back into the runtime from a host function, which is a can of worms that once opened exposes us to fun things like the possibility of a malicious program of overflowing the host's stack, etc. I think that if possible we should keep the model of "host calls don't call back into the runtime". As far as performance is concerned, #4 should be as fast or faster when optimally implemented (that is, if we guarantee that every host call will only have to cross the guest-host boundary twice at worst, and only once at the best). |
@koute I still have the same point of view, this does not conflict with RFC #4, and all current runtime apis rely on the method you say you don't like. I would like to emphasize again that the focus of this RFC is to extend the substrate runtime capabilities, not simply to improve performance. It does not prevent the implementation of RFC#4, it just provides another possibility |
Huh? But they don't call back into the guest? So can you clarify what you mean here? |
I mean the api defined by |
Sorry, I still don't know what you mean. (: The way the host calls currently work is that they don't call back into the runtime. The runtime calls the host function, the host function does its thing, and the control returns back to the host. Yes, the host does call into the runtime initially so technically you have a chain of "host -> guest -> host", but that only happens once. So for the sake of argument let's assume we implement this RFC. It will allow us to call the guest's allocator from the host. What is the use case here?
So I agree with @tomaka and I don't see how this RFC is orthogonal to #4; if we implement #4 then it pretty much makes this RFC pointless, unless you actually have an use case that could make use of this? |
Supports a pure wasm environment to verify blocks. Currently, if you want pure wasm to verify blocks, you need to replace all host functions, but alloc itself cannot be replaced. |
But this is also true after implementing #4 as far as I can see, because the host-side allocator will not be used at all anymore. |
This doesn't seem to hold true in the case of legacy host functions. |
...which is exactly the same as this RFC, no? (: Not that it matters, because the legacy host functions won't be used anymore for new blocks and will only be kept for sync compatibility. |
Maybe you're right. |
Closed since RFC #4 will be implmented. |
rendered
Related PR paritytech/polkadot-sdk#1658
BTW, I'm not a member of polkadot fellows, does this have any impact on RFC implementation?