Fix consensus error between wasm and native #1595
Conversation
cmichi
force-pushed
the
cmichi-fix-consensus-error-between-wasm-and-native
branch
5 times, most recently
from
57ec82f
to
6a9291e
Compare
There was a problem hiding this comment.
Seems ok to me!
It would be awesome if we could add some more diagnostic tools for catching this or other issues.
As we discussed today privately, we may want to improve the allocator with time, thus new bugs will be introduced anyway. Since the environment we deal with is not the easiest to debug one on one hand and on the other hand I'd consider the allocator to be very sensitive and rather not trivial it would be really good if we added some additional diagnostic tools, like fuzzing/quickchecking and things that will allow us to diagnose issues more easily.
I'd suggest you to check out the wee_alloc repo as it has some nice diagnostic tooling.
|
(To clarify: I don't expect this to be done in this PR) |
pepyakin
added
A6-seemsok
and removed
A0-please_review
|
Even 1K is likely too large a hit to take unless there's concrete evidence that any allocations much below that are few. I think we need to avoid the buddy allocator until there is a slab allocator available to ensure that the lowest leaves in the tree don't get exhausted through numerous allocations of small pieces of data. |
|
Just to be clear, until we have a compelling combination of slab+buddy allocator, I think we will need to keep the bump allocator. |
A bucket size of 8192 bytes is quite large and it turned out that this can exhaust the available heap space too too quickly. This is because even for allocating 1 byte a bucket of 8192 bytes is allocated/wasted.
The test didn't use an offset when setting up the heap. Hence the first successfully allocated pointer was always `0`. This is unfortunate since `0` is also the return value when there is an error. This lead to us not noticing that the test was failing, because it did not distinguish between success and error.
cmichi
force-pushed
the
cmichi-fix-consensus-error-between-wasm-and-native
branch
from
da5b4bb
to
9c3cc02
Compare
|
Alright, I have added a commit to this PR which reverts the current allocator to the linear allocator. In order to continue working on a better allocator we just need to revert Do we want to open #300 up again or create a new follow-up ticket? |
cmichi
added
A0-please_review
cmichi
force-pushed
the
cmichi-fix-consensus-error-between-wasm-and-native
branch
from
9c3cc02
to
5d127ce
Compare
cmichi
force-pushed
the
cmichi-fix-consensus-error-between-wasm-and-native
branch
from
5d127ce
to
df44a2e
Compare
|
Done: #1615 |
|
We may also consider trying out a free-list allocator. https://en.wikipedia.org/wiki/Free_list |
gavofyork
added
A8-looksgood
and removed
A0-please_review
|
FWIW I did an implementation of that ages ago (before custom allocators were a thing in rust): https://github.com/rphmeier/allocators/blob/master/src/freelist.rs maybe it can be ported over. |
|
Do we maybe want to make the allocator exchangable? |
|
was thinking that, but i think it's not needed for now, and could even be dangerous in terms of consensus. |
gavofyork
deleted the
cmichi-fix-consensus-error-between-wasm-and-native
branch
* Decrease bucket size A bucket size of 8192 bytes is quite large and it turned out that this can exhaust the available heap space too too quickly. This is because even for allocating 1 byte a bucket of 8192 bytes is allocated/wasted. * Return 0 if requested size too large * Improve test The test didn't use an offset when setting up the heap. Hence the first successfully allocated pointer was always `0`. This is unfortunate since `0` is also the return value when there is an error. This lead to us not noticing that the test was failing, because it did not distinguish between success and error. * Revert to linear allocator
* Decrease bucket size A bucket size of 8192 bytes is quite large and it turned out that this can exhaust the available heap space too too quickly. This is because even for allocating 1 byte a bucket of 8192 bytes is allocated/wasted. * Return 0 if requested size too large * Improve test The test didn't use an offset when setting up the heap. Hence the first successfully allocated pointer was always `0`. This is unfortunate since `0` is also the return value when there is an error. This lead to us not noticing that the test was failing, because it did not distinguish between success and error. * Revert to linear allocator
The bug from #1553 occurred because of two things playing into each other:
The bucket size of 8192 bytes is quite large, which caused the available heap space to be exhausted too soon (even for allocating just 1 byte a whole bucket of 8192 bytes needs to allocated).
This is an effect of the buddy allocator algorithm: There is less heap space available to be assigned, because a bucket does not necessarily have to be filled and thus the remaining bucket space is wasted.
The other thing was that there was a bug when trying to allocate in a full heap. A double allocation to the first pointer happened then. I adapted the tests so that they would have catched this.
Closes #1553