There's been a lot of talk about the Nimbus EVM "stack problem". I think we
assumed changing it would require big changes to the interpreter code, touching
a lot of functions.
It turned out to be a low hanging fruit.
This patch solves the stack problem, but hardly touches anything. The change
in EVM stack memory is from 13 MB worst case to just 48 kB, a 250x reduction.
I've been doing work on the database/storage/trie code. While looking at the
API between the EVM and the database/storage/trie, this stack patch stood out
and made itself obvious. As it's tiny, rather than more talk, here it is.
Note: This patch is intentionally small, non-invasive, and hopefully easy to
understand, so that it doesn't conflict with other work done on the EVM, and
can easily be grafted into any other EVM structure.
Motivation
==========
- We run out of space and crash on some targets, unless the stack limit is
raised above its default. Surprise segmentation faults are unhelpful.
- Some CI targets have been disabled for months due to this.
- Because usage borders on the system limits, when working on
database/storage/trie/sync code (called from the EVM), segmentation faults
occur and are misleading. They cause lost time due to thinking there's a
crash bug in the code being worked on, when there's nothing wrong with it.
- Sometimes unrelated, trivial code changes elsewhere trigger CI test failures.
It looks like abrupt termination. A simple, recent patch was crashing in
`make test` even though it was a trivial refactor. Turns out it pushed the
stack over the edge.
- A large stack has to be scanned by the Nim garbage collector sometimes.
Larger stack means slower GC and memory allocation.
- The structure of this small patch suggests how to weave async into the EVM
with almost no changes to the EVM, and no async transformation overhead.
- The patch seemed obvious when working on the API between EVM and storage.
Measurements before
===================
All these tests were run on Ubuntu 20.04 server, x86-64. This is one of the
targets that has been disabled for a while in CI in EVMC mode due to crashing,
and excessive stack usage is the cause.
Testing commit 0c34a8e3 `2021-04-08 17:46:00 +0200 CI: use MSYS2 on Windows`.
$ rm -f build/all_tests && make ENABLE_EVMC=1 test
$ ulimit -S -s 16384 # Requires larger stack than default to avoid crash.
$ ./build/all_tests 9 | tee tlog
[Suite] persist block json tests
...
Stack range 38496 depthHigh 3
...
Stack range 13140272 depthHigh 1024
[OK] tests/fixtures/PersistBlockTests/block1431916.json
These tests use 13.14 MB of stack to run, and so crash with the default stack
limit on Ubuntu Server 20.04 (8MB). Exactly 12832 bytes per EVM call stack
frame. It's interesting to see some stack frames take a bit more.
$ rm -f build/all_tests && make ENABLE_EVMC=1 test
$ ulimit -S -s 16384 # Requires larger stack than default.
$ ./build/all_tests 7 | tee tlog
[Suite] new generalstate json tests
...
Stack range 15488 depthHigh 2
...
Stack range 3539312 depthHigh 457
[OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest639.json
...
Stack range 3756144 depthHigh 485
[OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest458.json
...
Stack range 7929968 depthHigh 1024
[OK] tests/fixtures/eth_tests/GeneralStateTests/stCreate2/Create2OnDepth1024.json
These tests use 7.92MB of stack to run. About 7264 bytes per EVM call stack
frame. It _only just_ avoids crashing with the default Ubuntu Server stack
limit of 8 MB. However, it still crashes on Windows x86-64, which is why the
CI target is currently disabled.
On Linux where this passes, this is so borderline that it affects work and
testing of storage and sync code, because that's called from the EVM. Which
was a motivation for dealing with the stack instead of letting this linger.
Also, this stack greatly exceeds the default thread stack size.
$ rm -f build/all_tests && make ENABLE_EVMC=0 test
$ ulimit -S -s 16384 # Requires larger stack than default to avoid crash.
$ ./build/all_tests 9 | tee tlog
[Suite] persist block json tests
...
Stack range 33216 depthHigh 3
...
Stack range 11338032 depthHigh 1024
[OK] tests/fixtures/PersistBlockTests/block1431916.json
These tests use 11.33 MB stack to run, and so crash with a default stack limit
of 8MB. Exactly 11072 bytes per EVM call stack frame. It's interesting to see
some stack frames take a bit more.
$ rm -f build/all_tests && make ENABLE_EVMC=0 test
$ ulimit -S -s 16384 # Requires larger stack than default.
$ ./build/all_tests 7 | tee tlog
[Suite] new generalstate json tests
...
Stack range 10224 depthHigh 2
...
Stack range 2471760 depthHigh 457
[OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest639.json
...
Stack range 2623184 depthHigh 485
[OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest458.json
...
Stack range 5537824 depthHigh 1024
[OK] tests/fixtures/eth_tests/GeneralStateTests/stCreate2/Create2OnDepth1024.json
These tests use 5.54 MB of stack to run, and avoid crashing on with a default
stack limit of 8 MB. About 5408 bytes per EVM call stack frame.
However, this is uncomfortably close to the limit, as the stack frame size is
sensitive to changes in the code.
Also, this stack greatly exceeds the default thread stack size.
Measurements after
==================
(This patch doesn't address EVMC mode, which is not our default. EVMC stack
usage remains about the same. EVMC mode is addressed in another tiny patch.)
$ rm -f build/all_tests && make ENABLE_EVMC=0 test
$ ulimit -S -s 80 # Because we can! 80k stack.
$ ./build/all_tests 9 | tee tlog
[Suite] persist block json tests
...
Stack range 496 depthHigh 3
...
Stack range 49504 depthHigh 1024
[OK] tests/fixtures/PersistBlockTests/block1431916.json
$ rm -f build/all_tests && make ENABLE_EVMC=0 test
$ ulimit -S -s 72 # Because we can! 72k stack.
$ ./build/all_tests 7 | tee tlog
[Suite] new generalstate json tests
...
Stack range 448 depthHigh 2
...
Stack range 22288 depthHigh 457
[OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest639.json
...
Stack range 23632 depthHigh 485
[OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest458.json
...
Stack range 49504 depthHigh 1024
[OK] tests/fixtures/eth_tests/GeneralStateTests/stCreate2/Create2OnDepth1024.json
For both tests, a satisfying *48 bytes* per EVM call stack frame, and EVM takes
not much more than 48 kB. With other overheads, both tests run in 80 kB stack
total at maximum EVM depth.
We must add some headroom on this for database activity called from the EVM,
and different compile targets. But it means the EVM itself is no longer a
stack burden.
This is much smaller than the default thread stack size on Linux (2MB), with
plenty of margin. It's even smaller than Linux from a long time ago (128kB),
and some small embedded C targets. (Just fyi, though, some JVM environments
allocated just 32 kB to thread stacks.)
This size is also well suited to running EVMs in threads, if that's useful.
Subtle exception handling and `dispose`
=======================================
It is important that each `snapshot` has a corresponding `dispose` in the event
of an exception being raised. This code does do that, but in a subtle way.
The pair of functions `execCallOrCreate` and `execCallOrCreateAux` are
equivalent to the following code, where you can see `dispose` more clearly:
proc execCallOrCreate*(c: Computation) =
defer: c.dispose()
if c.beforeExec():
return
c.executeOpcodes()
while not c.continuation.isNil:
c.child.execCallOrCreate()
c.child = nil
(c.continuation)()
c.executeOpcodes()
c.afterExec()
That works fine, but only reduces the stack used to 300-700 kB instead of 48 kB.
To get lower we split the above into separate `execCallOrCreate` and
`execCallOrCreateAux`. Only the outermost has `defer`, and instead of handling
one level, it walks the entire `c.parent` chain calling `dispose` if needed.
The inner one avoids `defer`, which greatly reduces the size of its stackframe.
`c` is a `var` parameter, at each level of recursion. So the outermost proc
sees the temporary changes made by all inner calls. This is why `c` is updated
and the `c.parent` chain is maintained at each step.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
* switch to chronos metrics, remove insecure
See https://github.com/status-im/nimbus-eth2/pull/2468
also fixes pcre linking for real, and adds some random build flags that
help nimbus-eth2 stay afloat
* fix help
* don't omit frame pointers on windows
The "new block chain json tests" were being skipped on Linux, but silently so
that CI didn't notice. These are a significant part of the Ethereum test suite.
See the missing output from `make test`, also visible in CI logs for Linux
targets (prior to this commit):
[OK] tests/fixtures/eth_tests/TransactionTests/ttGasPrice/TransactionWithGasPriceOverflow.json
[OK] tests/fixtures/eth_tests/TransactionTests/ttGasPrice/TransactionWithHighGasPrice.json
[Suite] new block chain json tests
<-- nothing here
[Suite] Fork ID tests
[OK] MainNet
[OK] RopstenNet
Commit 3d468a7 (`fixes path pointing to eth_tests`) renamed the JSON fixture
source directoryf in the witness-builder tests but not the regular blockchain
tests. As a result, searching for JSON test files yielded zero results. To
make this less likely in future, zero results is now an error.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Commit 3d468a7 (`fixes path pointing to eth_tests`) renamed a test output
file subtly, changing just the capitalisation:
- jsonTest(newFolder, "newBlockChainTests", testFixture, skipNewBCTests)
+ jsonTest(newFolder, "newBlockchainTests", testFixture, skipNewBCTests)
However, the file checked into the repo continued to be the old name, uppercase
`C`. Of course, Mac and Windows have a sort of case insensitivity and Linux
does not.
This meant, on Linux, both files were created, test differences didn't make
themselves visible in `git diff`, and the repo would not get updates with
changed test output. On Mac and Windows it worked.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Prior to this commit, when using discv4 (Kademlia) to find peers, there is a
crash after a few minutes. It occurs for most of us on Eth1 mainnet, and
everyone on Ropsten.
Tested for about 4 days constant operation on Ropsten. The crash which occured
every few minutes no longer occurs, and discv4 keeps working.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
why:
it was convenient to have relocatable source modules when writing the
vm interface wrappers. this patch moves it back to the standard.
also:
there are no deep links into the vm folder anymore which leaves some
room for manoeuvring inside
why:
the nvm_ prefix was used inside the vm folder to hide them temporarily
from the outside world while writing export wrappers. now all
functionality is accessed via vm_*, rather than vm/* imports.
todo:
at a later stage the import headers of the vm modules need to get fixed
to meet style guide standards (as jacek kindly pointed out.)
why:
relative paths make sources inherently non-relocatable
details:
import base is set to the nimbus directoy, so importing ./stack
from file interpreter.nim becomes vm/stack etc.
caveat:
a file named nimbus/strformat.nim would clash with strformat (but
not with std/strformat)
why:
under win/mingw32 the --docRoot argument value for the NIM doc
generator needs a window-ish path C:\\MinGW\\msys\\1.0\\home\\...
also:
fix clean up for nimdoc.out.css or nimdoc_out.css (varies on
win/posix operating system)
why:
* easy browsing of prototype docs, allows to follow links with
a web browser on the local file system
* some md & png files may contain additional documentation
overview:
* separate nimbus/makefile, try "make -C nimbus" for instructions
* running "make -C nimbus docs" will do the job
* x-ref file in nimbus/docs/theindex.html
* additional md and png files in nimbus/docs/ex/.. subdirectory
details:
* a newer nim compiler provides better referencing when available, in
particular the back link to the indices are not provided by the 1.2.10
nim compiler (automatically handled by makefile)
* make patterns are used to update files only when the timestamp changes
* should provide "discount" markdown generator, otherwise fallback
to <pre/> encapsulated text file
Ștefan Talpalaru
Jamie Lokier
Jacek Sieka
Jordan Hrycaj
jangko
kdeme
Jordan Hrycaj