This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WASM Local-blob override #7317
WASM Local-blob override #7317
Changes from 5 commits
aa4a698
2119c33
ad6a811
087e797
77134e1
f030c7e
770d825
a54ae38
0cf1263
c67b981
7a83168
b04a260
7704458
f80b33c
d1254c5
ed19163
3feed25
bdd847d
73efeb6
2675653
474c442
8a51780
a9f8e85
2aab45e
cd7a3e3
2895bc4
f9fac2b
0e4e972
abc48f7
8f4c857
32bd6db
6a50eee
a0c1f55
f940c71
26975d3
8433731
3e8d18f
d14e1d7
7b99336
626e453
fed565c
5812b10
d40d5bb
693f506
a2a41de
824b24e
0506100
82e2683
554f68e
2ddf6a0
18589c7
5781c1e
1401b18
37266bf
1ef18ab
551f3ed
54969dc
25dc83f
7d3ee25
e654dbe
665e20c
5a91f0d
b2b9ba5
103f149
a84dcbf
1ac97cb
6cc0a8f
8503689
8fc367d
f90db2f
1b355b7
427d9ae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think this feels a bit clunky and I wonder if we can come up with a smoother API. If
wasm_overwrite
could take only thespec_version
maybe we could get away withtry_replace(spec_v, &state)
? It also feels weird to me thattry_replace
returns the inputcode
if no override was found; can't we move that check elsewhere, perhaps intoruntime_code()
?Maybe if you expanded a bit on the motivation it'll be clearer to me why it needs to be like this.
Having something like this would be nicer imo, (pseudo-code):
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.
Changed this so that now WasmOverwrite just has a
get
method that returnsNone
if it doesn't find an overwrite, where LocalCallExecutor gets the version for a blob, so it looks something likeI think it's a bit odd that it's accepting a runtime code and then unwrapping into another runtime code, but it encapsulates calling for the version into it's own function on LocalCallExecutor now, where the actual
get
function only needsspec_version
andheap_pages
. We could go further and put thecheck_overwrite
inWasmOverwrite
toocheck_overwrite
accepts RuntimeCode again, for the convenience of havingcode.heap_pages
. We could achieve a more similar API to the comment by using theruntime_version
method on LocalCallExecutor and then gettingheap_pages
before callingget
and would probably look more likeThe reason to avoid calling
self.runtime_version
on LocalCallExecutor now incheck_overwrites
is to avoid requiring another type parameter (Block: BlockT
) on all LocalCallExecutor types.Second version is way more clear on what's happening but first requires less code wherever an overwrite is required