Skip to content
This repository has been archived by the owner on Mar 5, 2024. It is now read-only.

sc handler optimisations #1396

Merged
merged 3 commits into from
Jun 24, 2022
Merged

Conversation

andymck
Copy link
Contributor

@andymck andymck commented Jun 20, 2022

Provides optimisations/improvements of the state channel GRPC handler and the shared blockchain_state_channel_common module. This mod is shared across both grpc and libp2p transport layers for state channel requests.

Optimisation/improvements include:

  • Remove cached versions of the blockchain and ledger in the state channel handler state
  • Add a simple ETS cache to blockchain_worker to hold the current blockchain. Avoids blocking calls to the worker by spawned grpc handlers.

@@ -387,6 +422,7 @@ init(Args) ->
[ libp2p_swarm:listen(SwarmTID, Addr) || Addr <- ListenAddrs ]),
NewState = #state{swarm_tid = SwarmTID, blockchain = Blockchain,
gossip_ref = Ref},
true = ets:insert(?CACHE, {?CHAIN, Blockchain}),
Copy link
Contributor

Choose a reason for hiding this comment

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

should be pretty easy to move this into a handle_continue/2 so we load the chain into the cache table before receiving any other messages post-init. would it make sense (perhaps later) to consolidate these "blockchain worker cache items" into a single shared globally readable cache, owned and updated by the worker, and consolidate the "current height" cache that's in review to merge into the txn mgr? any others doing similar work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we def need to get loading the chain out of the init, using handle_continue would work well there. I dont want to address that in this PR as its dramatically increases the impact and thus the risk.

We can address in a follow up PR and that could also include replacing all blocking calls to the worker to return the chain with the new non blocking call. Extending the cache to also include the height and additional data does make sense too.

@andymck andymck force-pushed the andymck/optimise-core-sc-server-handler branch from 5036b49 to 831d4cc Compare June 21, 2022 13:38
@andymck andymck marked this pull request as ready for review June 21, 2022 13:38
@andymck andymck force-pushed the andymck/optimise-core-sc-server-handler branch from 831d4cc to ee9ae87 Compare June 24, 2022 10:05
@@ -506,6 +545,7 @@ handle_call({install_aux_snapshot, Snapshot}, _From,
{ok, GossipRef} = add_handlers(SwarmTID, NewChain),
notify({new_chain, NewChain}),
blockchain_lock:release(),
true = ets:insert(?CACHE, {?CHAIN, NewChain}),
Copy link
Contributor

Choose a reason for hiding this comment

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

don't think that we need this one here, this is for the aux ledger.

@@ -538,16 +578,19 @@ handle_call({add_commit_hook, CF, HookIncFun, HookEndFun} , _From, #state{blockc
Ledger = blockchain:ledger(Chain),
{Ref, Ledger1} = blockchain_ledger_v1:add_commit_hook(CF, HookIncFun, HookEndFun, Ledger),
Chain1 = blockchain:ledger(Ledger1, Chain),
true = ets:insert(?CACHE, {?CHAIN, Chain1}),
Copy link
Contributor

Choose a reason for hiding this comment

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

nor here

{reply, Ref, State#state{blockchain = Chain1}};
handle_call({add_commit_hook, CF, HookIncFun, HookEndFun, Pred} , _From, #state{blockchain = Chain} = State) ->
Ledger = blockchain:ledger(Chain),
{Ref, Ledger1} = blockchain_ledger_v1:add_commit_hook(CF, HookIncFun, HookEndFun, Pred, Ledger),
Chain1 = blockchain:ledger(Ledger1, Chain),
true = ets:insert(?CACHE, {?CHAIN, Chain1}),
Copy link
Contributor

Choose a reason for hiding this comment

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

nor here

{reply, Ref, State#state{blockchain = Chain1}};
handle_call({remove_commit_hook, RefOrCF} , _From, #state{blockchain = Chain} = State) ->
Ledger = blockchain:ledger(Chain),
Ledger1 = blockchain_ledger_v1:remove_commit_hook(RefOrCF, Ledger),
Chain1 = blockchain:ledger(Ledger1, Chain),
true = ets:insert(?CACHE, {?CHAIN, Chain1}),
Copy link
Contributor

Choose a reason for hiding this comment

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

nor here

@@ -743,6 +787,7 @@ handle_info({'DOWN', RocksGCRef, process, RocksGCPid, Reason},
{noreply, State#state{rocksdb_gc_mref = undefined}};

handle_info({blockchain_event, {new_chain, NC}}, State) ->
true = ets:insert(?CACHE, {?CHAIN, NC}),
Copy link
Contributor

Choose a reason for hiding this comment

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

pretty sure this is duplicative

Copy link
Contributor

@evanmcc evanmcc left a comment

Choose a reason for hiding this comment

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

I'm worried about how many times we insert the chain into the cache. I've added some that I think are duplicated, but I think that someone should add logging to each of these inserts and make sure that we're only calling it once on any given code path. the important ones are:

  1. Startup with existing chain
  2. Load new genesis
  3. load snapshot

@andymck
Copy link
Contributor Author

andymck commented Jun 24, 2022

So the approach I took was to insert to the cache anytime the chain in state is being updated

Copy link
Contributor

@evanmcc evanmcc left a comment

Choose a reason for hiding this comment

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

my mistake, this is fine

@evanmcc evanmcc merged commit 5a00a4b into master Jun 24, 2022
@evanmcc evanmcc deleted the andymck/optimise-core-sc-server-handler branch June 24, 2022 21:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants