-
Notifications
You must be signed in to change notification settings - Fork 234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Gb 3355 parallel tx eval 2 #3415
base: master
Are you sure you want to change the base?
Conversation
, calls = C | ||
, channels = Ch | ||
, contracts = Co | ||
, ns = Ns |
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.
, ns = Ns | |
, ns = Ns |
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.
- Doesn't fully work - Failing test #3424 shows an failing test case
- When restarting a worker due to dependencies we redo the (probably) already done signature check - By running the tests with {dont_verify_signature, true} the overhead between parallel and sequential TX processing becomes more reasonable
- I don't see the option of passing the Eval CTX from the outside - If parallel tx eval got enabled by default then we will build the deps before entering aec_conductor.
- I think we might enable this by default in case we implement [RFC] Delegate block postprocesing to a seperate worker #3419 - the preprocesing will be done while other blocks are being inserted - minimizing the hot path :)
par_apply_txs_on_state_trees(SignedTxs, Valid, Invalid, Trees, Env, Opts) -> | ||
ParMaxDeps = proplists:get_value(par_max_deps_ratio, Opts, undefined), | ||
case aec_trees_proxy:prepare_for_par_eval(SignedTxs) of | ||
#{tx_count := TxC, max_dep_count := DepC} when (DepC / TxC) > ParMaxDeps -> |
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.
Atoms always are bigger than floats/integers. When the ratio is undefined then (DepC/TxC) > undefined
is always false so essentially when par_max_deps_ratio is undefined this will always evaluate the transactions in parallel. Please add a comment about it.
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.
Yes, this was a a deliberate trick to short-circuit that check. Should have added a comment about it, of course. :)
end; | ||
{'DOWN', MRef, _, _, Error} -> | ||
{error, Error} | ||
after 5000 -> |
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.
Is this timeout OK for an HDD?
gen_server:start_monitor(?MODULE, {self(), Trees, Env, Ctxt}, []). | ||
-else. | ||
start_monitor(Trees, Env, #{indexed := _} = Ctxt, Opts) -> | ||
_TStore = get_tstore(), |
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.
Looks like it can be removed.
{ok, {Pid, _, SignedTx} = Worker} -> | ||
?event({restarting_worker, Ix, Pid}), | ||
kill_worker(Worker), | ||
{Cs, Pids} = start_worker( |
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.
This will redo the signature check - checking the signature doesn't require DB access - If a worker happens to be restarted 10 times the signature check will be done 10 times :(
@@ -13,6 +13,10 @@ | |||
-define(TEST_MODULE, aec_trees). | |||
-define(MINER_PUBKEY, <<42:?MINER_PUB_BYTES/unit:8>>). | |||
|
|||
-define(OPTS, [{line, ?LINE}, {fname, ?FUNCTION_NAME}]). | |||
|
|||
-define(SOPHIA_IRIS_FATE, 6). |
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.
There is a header file with those defines.
@@ -738,8 +835,160 @@ ct_create_tx(Sender, VmVersion, ABIVersion) -> | |||
{ok, Tx} = aect_create_tx:new(Spec), | |||
Tx. | |||
|
|||
apply_txs_on_state_trees(SignedTxs, TreesIn, Env, Opts) -> | |||
T0 = erlang:system_time(microsecond), | |||
SeqRes = ?TEST_MODULE:apply_txs_on_state_trees(SignedTxs, TreesIn, Env, Opts), |
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 would rather use timer:tc/1
:)
{IsValid, Events} = | ||
case Reason of | ||
{ok, NewEvents} -> | ||
{true, Events0 ++ NewEvents}; |
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.
This is linear in the first list - this might become an issue when many events were emitted
Failing test Merging this for the day we extract the useful parts of parallel Tx evaluation that could be used for lazy block fetching
Left open so we can extract the block witness in case we consider it useful. |
See issue #3355. This PR attempts to (eventually) speed up block evaluation by processing heavy transactions in parallel.
Currently, the implementation has been tested with spend txs, either accessing shared resources (same account) or being independent. The
aehttp_contract_SUITE
also double-checks with parallel eval (via dry-run over HTTP), but using only singleton lists of contract call transactions.A preliminary performance test indicates that for now, the overhead of parallel eval is roughly 10x that of sequential eval. This should be possible to reduce, e.g. by removing the
lager:debug()
calls, but it is clear that e.g. parallel eval ofspend
txs will never pay off, since these basically only update the state trees. The expectation is that especially complex contract evaluations will benefit from parallel evaluation.The tracing improvements rely on the uwiger/trace_runner#3 PR. The commit hash will change once that PR is merged.