Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Gb 3355 parallel tx eval 2 #3415

wants to merge 8 commits into from

Conversation

uwiger
Copy link
Member

@uwiger uwiger commented Nov 26, 2020

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 of spend 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.

, calls = C
, channels = Ch
, contracts = Co
, ns = Ns
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
, ns = Ns
, ns = Ns

Copy link
Contributor

@gorbak25 gorbak25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Doesn't fully work - Failing test #3424 shows an failing test case
  2. 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
  3. 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.
  4. 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 ->
Copy link
Contributor

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.

Copy link
Member Author

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 ->
Copy link
Contributor

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(),
Copy link
Contributor

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(
Copy link
Contributor

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).
Copy link
Contributor

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),
Copy link
Contributor

@gorbak25 gorbak25 Dec 9, 2020

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};
Copy link
Contributor

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

@uwiger uwiger mentioned this pull request Nov 12, 2021
Failing test

Merging this for the day we extract the useful parts of parallel Tx evaluation that could be used for lazy block fetching
@velzevur
Copy link
Member

Left open so we can extract the block witness in case we consider it useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants