From 877074aaf46b0f703f665458eb4cd85b2a8d13ee Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Sat, 25 Mar 2023 12:07:37 +0000 Subject: [PATCH] Dont do locking using defer --- chain/stmgr/execute.go | 23 ++++++++++++++--------- chain/stmgr/stmgr.go | 4 +++- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/chain/stmgr/execute.go b/chain/stmgr/execute.go index 7613989283..8c85a1031b 100644 --- a/chain/stmgr/execute.go +++ b/chain/stmgr/execute.go @@ -130,15 +130,16 @@ func (sm *StateManager) ExecutionTraceWithMonitor(ctx context.Context, ts *types func (sm *StateManager) ExecutionTrace(ctx context.Context, ts *types.TipSet) (cid.Cid, []*api.InvocResult, error) { tsKey := ts.Key() - { - // check if we have the trace for this tipset in the cache - sm.execTraceCacheLock.Lock() - defer sm.execTraceCacheLock.Unlock() - if entry, ok := sm.execTraceCache.Get(tsKey); ok { - // we have to make a deep copy since caller can modify the invocTrace - return entry.postStateRoot, makeDeepCopy(entry.invocTrace), nil - } + // check if we have the trace for this tipset in the cache + sm.execTraceCacheLock.Lock() + if entry, ok := sm.execTraceCache.Get(tsKey); ok { + // we have to make a deep copy since caller can modify the invocTrace + // and we don't want that to change what we store in cache + invocTraceCopy := makeDeepCopy(entry.invocTrace) + sm.execTraceCacheLock.Unlock() + return entry.postStateRoot, invocTraceCopy, nil } + sm.execTraceCacheLock.Unlock() var invocTrace []*api.InvocResult st, err := sm.ExecutionTraceWithMonitor(ctx, ts, &InvocationTracer{trace: &invocTrace}) @@ -146,7 +147,11 @@ func (sm *StateManager) ExecutionTrace(ctx context.Context, ts *types.TipSet) (c return cid.Undef, nil, err } - sm.execTraceCache.Add(tsKey, tipSetCacheEntry{st, makeDeepCopy(invocTrace)}) + invocTraceCopy := makeDeepCopy(invocTrace) + + sm.execTraceCacheLock.Lock() + sm.execTraceCache.Add(tsKey, tipSetCacheEntry{st, invocTraceCopy}) + sm.execTraceCacheLock.Unlock() return st, invocTrace, nil } diff --git a/chain/stmgr/stmgr.go b/chain/stmgr/stmgr.go index d7eabaaec1..91284b83a2 100644 --- a/chain/stmgr/stmgr.go +++ b/chain/stmgr/stmgr.go @@ -142,7 +142,9 @@ type StateManager struct { // We keep a small cache for calls to ExecutionTrace which helps improve // performance for node operators like exchanges and block explorers - execTraceCache *lru.ARCCache[types.TipSetKey, tipSetCacheEntry] + execTraceCache *lru.ARCCache[types.TipSetKey, tipSetCacheEntry] + // We need a lock while making the copy as to prevent other callers + // overwrite the cache while making the copy execTraceCacheLock sync.Mutex }