Encoding v1 for Configurables #5942
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xunilrj
force-pushed
the
xunilrj/new-encoding-configurables
branch
from
6c90385
to
242f8d0
Compare
Benchmark for 3ab0c9bClick to view benchmark
|
Benchmark for 220b67aClick to view benchmark
|
Benchmark for 520aae7Click to view benchmark
|
Benchmark for cb6635fClick to view benchmark
|
Benchmark for 1a12428Click to view benchmark
|
Benchmark for 83f12afClick to view benchmark
|
Benchmark for 78c35f3Click to view benchmark
|
Benchmark for fac8e06Click to view benchmark
|
xunilrj
force-pushed
the
xunilrj/new-encoding-configurables
branch
from
0b0fae8
to
2e5041e
Compare
Benchmark for 5836b4dClick to view benchmark
|
Benchmark for 47a7010Click to view benchmark
|
Benchmark for 1f0e379Click to view benchmark
|
Benchmark for 48efa0dClick to view benchmark
|
IGI-111
reviewed
May 6, 2024
Benchmark for ce45538Click to view benchmark
|
xunilrj
force-pushed
the
xunilrj/new-encoding-configurables
branch
from
23d1c8f
to
d0d9b04
Compare
Benchmark for f0772f4Click to view benchmark
|
ironcev
requested changes
May 7, 2024
There was a problem hiding this comment.
Before reviewing the implementation, there are two points that IMO needs to be addressed: the serious change in the semantics of configurables as entities and the cost of usage.
Semantics
Up to now, configurables were treated like other constants, with the difference being deploy-time defined. But unchangable at runtime and in every aspect same as other constants at runtime.
This means, considering:
configurable {
CONF: SomeStruct = SomeStruct { x: 0, y: 0 },
}
fn fun() {
let p_1 = _addr_of(CONF);
let p_2 = _addr_of(CONF);
let r_1 = &CONF:
let r_2 = &CONF:
}
the two pointers were pointing to the same struct, same as the two references.
With the current v1 implementation the code becomes, simplified to demonstrate the new semantics:
fn fun() {
let p_1 = _addr_of(abi_decode(CONF));
let p_2 = _addr_of(abi_decode(CONF));
let r_1 = &abi_decode(CONF):
let r_2 = &abi_decode(CONF):
}
This gives us four different entities, means different pointers and different references.
Cost of usage
Calling abi_decode on every use explodes the generated IR code tremendously. The simple line from above:
let p_1 = _addr_of(CONF);
used to be just one IR instruction (store c0 to v0). Understandably it is now dozens of costly instructions like function calls, memcpy, asm, ...
This makes simple expressions like CONF.x + CONF.y extremely expensive at run-time, prohibits every function using configurables to be inlined because it will be huge etc. E.g., just having CONF used in a loop could end up in a huge gas consumption.
Mitigation
I am thinking how to mitigate both points, to keep the proper semantics and also to remove the hidden runtime cost because it is huge and on every call site.
Could we perhaps do the decoding only once in the __entry and also store the decoded values in the config section and then use them as we use them now, as direct pointers? I am not sure if we can write to config from the code, but if possible, the config would look like e.g.:
// Encoded values.
c0 = config [u8; 16] [u8 0, u8 0, u8 0, u8 0, u8 0, u8 0, u8 0, u8 0, u8 0, u8 0, u8 0, u8 0, u8 0, u8 0, u8 0, u8 0],
c1 = config [u8; 8] [u8 0, u8 0, u8 0, u8 0, u8 0, u8 1, u8 182, u8 105],
...
// Their decoded counterparts filled in `__entry`.
c10 = config { u64, u64 } .... some value here ....,
c11 = config u64 112233,
We can explore other options too.
I see it as necessary to resolve the above two issues, semantics and cost, before getting the feature out.
|
I agree, but the question is if makes sense to merge this PR as is and improve these things later. Because the SDK needs this PR to finish their migration to encoding V1. |
|
I am fine with merging as is to unblock the SDK team as long as we continue immediately working on the points. Because as mentioned having the feature out as is would have serious unwanted consequences. Also, I can ignore configurables in my work in properly treating consts everywhere in IR and in inlining until the above issues are solved that also shouldn't be a problem. |
This is the idea. |
ir generation for intrinsics intrinsics compilation to ir IR generation for encoding intrinsics working configurable encoding bytes as arrays uints working configurable_const test ok append for string array more tests passing fmt and clippy issue support for string slice as configurable don´t allow storage to be initialized with configurables fmt core lib clippy issues fix uints smaller than u64 clippy issues fixing configurable encoding for enums fix enum encoding more size hints implemented more tests fixing CI issues
Co-authored-by: Igor Rončević <ironcev@hotmail.com>
xunilrj
force-pushed
the
xunilrj/new-encoding-configurables
branch
from
e4b9542
to
4252fe2
Compare
Benchmark for ded4b0dClick to view benchmark
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This PR uses
AbiEncodeon configurables. We can imagine thatwill be desugared into
To allow this, now the whole
encodefunction and all trait impls run insideconst_eval. To make this work, three new intrinsic were implemented:EncodeBufferEmpty,EncodeBufferAppend, andEncodeBufferAsRawSlice.EncodeBufferEmptycreates an empty "encoding buffer", which is composed of a pointer to the buffer, the buffer capacity, and how many bytes were written already.EncodeBufferAppendappends any primitive data types to the buffer. This intrinsic does not mutate its argument, it returns a new "encoding buffer". If no reallocation is needed, the pointer and the capacity stay the same.EncodeBufferAsRawSlicereturns a slice with is composed of the buffer pointer and its length (not the capacity).Errors
Some constant expressions cannot live inside the data section because their encoding depends on some instance value, for example: string slices, Vecs, Sttrings, Bytes etc... For these we now we return an error in these cases, like
Future TODOs
This PR adds two more tasks to #5727:
Checklist
Breaking*orNew Featurelabels where relevant.