Add possibility of making static buffer configurable #1471
Comments
|
@cmichi rust stable does not support generic parameters to be used in const generics. I have to fallback to passing the constant explicitly across the whole codebase: pub struct StaticBuffer<const CAPACITY: usize> |
|
The problem doesn't seem to be trivial because |
|
After digging, what we can do to tackle the issue is to use constant generics for +/// A static buffer with `CAPACITY` kB of capacity.
+pub struct StaticBuffer<const CAPACITY: usize> {
/// The static buffer with a total capacity of 16 kB.
- buffer: [u8; Self::CAPACITY],
+ buffer: [u8; CAPACITY],
}then upon instantiation of impl<E: Environment> OnInstance<E> for EnvInstance {
fn on_instance<F, R>(f: F) -> R
where
F: FnOnce(&mut Self) -> R,
{
// not a final version, just an idea
extract_value!(E::CAPACITY, {
static mut INSTANCE: EnvInstance = EnvInstance {
// `VALUE` would be replaced with am int literal specified in `E::CAPACITY`
buffer: StaticBuffer::<VALUE>::new(),
};
f(unsafe { &mut INSTANCE })
}
}
} |
|
Update. |
|
We have recently encountered the 16K limitation in several scenarios. Is there a development plan to address this issue? |
There are currently other more important issues on the roadmap such as OpenZeppelin issues. Once those are cleared, I plan to get back to tacking this issue. Because of tight dependency coupling, most likely, the bus will have to be configured in the ink! smart contract directly via the macro. We need to assess security implications of that design first. |
|
There are legitimate use cases of sending data of around 53Kb to ink! contracts. Would the configuration afforded by this change support that amount? |
|
I started experimenting with the other possibilities to implement the feature and there are a few points:
|
|
After some digging, I came up with 2 possible PRs to address the issues. The #1869 modifies the size via the environmental variable, and the #1872 uses features to select the size from the predefined set of values (originally used by @kvinwang). I am interested in the opinions of which one is more idiomatic. I discarded the option of messing with the codegen. The static buffer is created with |
|
How about using a |
static buffer is frequently used in almost every part of ink! execution pipeline, we need to keep it optimised. Replacing it with anything else will cause performance degradation |
If that's the case, we can implement a buffer where the front half is a static array, and the second half is a Vec. Theoretically, this will not degrade performance. something like the TinyVec |
|
More clear: |
This sounds like very complicated and inefficient solution. The whole idea of using There is another issue I described earlier: the order of dependencies. |
|
I didn't mean to store the buffer in two separate locations. What I meant is that from the perspective of the type system, we can create a type like: pub struct LowerBoundVec<const L: usize, T> {
buffer: Vec<T>,
}
impl<const L: usize, T> LowerBoundVec<L, T> {
fn index(&self, i: usize) -> &T {
if i < L {
unsafe { self.buffer.get_unchecked(i) }
} else {
&self.buffer[i]
}
}
}The inner buffer is logically divided into two parts: a fixed array at the beginning and a dynamic tail, all stored in continuous memory located on the heap. When the front part is accessed, such as when decoding/encoding small fixed-size data types, it provides the compiler with more information that an overflow won't occur. As a result, the compiler can optimize away the bound checking, leading to performance that is not much different from using a static buffer. |
|
With the design you propose in places like https://github.com/paritytech/ink/blob/02a0bf92566acc61593c1bdad6c299d0fd6ff649/crates/env/src/api.rs#L69 What we can do is to use lazy static with the |
It is not necessary to use lazy static. We can setup the static buffer with a non-const size in the two entry
It won't introduce any extra generic parameters. Changes just keep inside the static buffer. It just change: pub struct StaticBuffer {
buffer: [u8; 16k],
}to: pub struct StaticBuffer {
buffer: LowerBoundVec<16k, u8>,
} |
|
As you said about this structure:
This is the main problem. This is performance digression. |
|
No matter the buffer is on heap or BSS, there should be not too much difference at the low level wasm asm. Both need to alloc a piece of zeroed memory. However, I tried to bench it with patched ink with a Using StaticBuffer with Using StaticBuffer with I am confused by the result. Both get exectly the same gas consumed. There should be some problem in the gas metering. |
|
Most likely there is a mistake in your benchmark. The reason is that it is practically unlikely that the compiler optimised |
Yes, I finally found it was a mistake in my test. I have just temporarily modified Phala blockchain's e2e code for testing purposes, so there is no reproducer code available. However, I found the reason of the identical result was that Phala's worker always mask the lower 5% bits of the returned gasConsumed in the with with where #[ink(message)]
pub fn runtime_version(&self) -> (u32, u32) {
pink::ext().runtime_version()
}
#[ink(message)]
pub fn on_block_end_called(&self) -> bool {
self.on_block_end_called
}and the There are indeed some expected overheads. But the result looks acceptable in our projects. We can place the dynamic buffer under a cargo feature gate named As for #1869, if the environment var would be hidden behind the cargo-contract, it would be fine. Maybe cargo-contract can read it from some custom section in the Cargo.toml rather than introducing a new |
This lies on a more subjective side of things. What I fear with introducing custom tuning of things like StaticBuffer is that ink! may become "over-configurable" framework like Substrate. The whole idea with ink! is simplicity in usage and its opinionated nature. If you need precisely it is kinda similar to buying a laptop if you run an application that consumes 18GB of RAM, you don't request a custom build with 20GB of RAM from a manufacturer, you just buy a laptop with 32GB of RAM and has extra RAM for any other future use cases. |
Because on Phala, a contract can use up to 4MB of RAM.
Can you imagine someone want to create a ramdisk on a laptop with 4GB RAM, he only have choices 512M, 1GB, 16GB ...? |
I am happy to add it as one of the configuration options: i.e.
|
Adding 2MB won't help anything. The main reason we want a larger buffer is that our contract support simple http request, and the response would be passed through the static buffer. If we config the buffer to 2MB, the heap would get less than 2MB, then the contract won't even be able to parse a 800K response. Because the raw response itself has to be copied to heap, and need more memory to parse the raw response. |
|
Sorry, I fail to understand the logic here. Why do you exactly need 1.2MB but not 2MB? If 2MB is too small for you, configure the buffer to be 4MB? |
2MB is too large because it would shrink the heap size to less than 2MB. Suppose we have the following code in a ink method: let response = http_get(url_of_the_json)?; // How large does the response we support?
let data: Data = Decode::decode(&mut &response.body)?;If we config the buffer to 2MB, then the With this layout:
If we config the buffer to 1MB, we have this layout: With this layout, there is a wasted space in the heap. This is why we set the io buffer size to about 1.2 MB, because it would support max of 1.2M http response. |
|
Resolved by #1869 |
We should make the size of the static buffer configurable. This is what #1279 tried to do. See here for an explanation what the static buffer is.
The ideal approach for implementing this would be to add a property to our
Environmenttrait:/// The environmental types usable by contracts defined with ink!. pub trait Environment { + /// TODO + const CAPACITY: u8; +This will require adding the
E: Environmentbound in all kinds of places, so that accessingE::CAPACITYbecomes possible:Some necessary modifications:
impl OnInstance for EnvInstance { - fn on_instance<F, R>(f: F) -> R + fn on_instance<E, F, R>(f: F) -> R where F: FnOnce(&mut Self) -> R, + E: Environment, {There's a ton more places where we'll need to pass the type.
The text was updated successfully, but these errors were encountered: