tokens: eliminate commit-path cache barriers, struct copying, and unguarded log allocations - #2364
SurbhiAgarwal1 wants to merge 3 commits into
Conversation
Effi-S
added
performance
improvements
storage
benchmark
cleanup
services
and removed
improvements
labels
07555dc to
2d0ca3c
Compare
AkramBitar
left a comment
AkramBitar
left a comment
There was a problem hiding this comment.
Review of 541179e8c..2d0ca3c53 (the localSession race fix + the tokens commit-path optimizations).
Verified before commenting: go build, go vet and go test are clean on all touched packages, plus -race -count=200 on the new session tests. I could not reproduce a close of closed channel / send on closed channel panic: writeChannel is closed only when closed && inFlight == 0, and inFlight is incremented under the same lock that rejects new sends, so that window is properly closed. Left and right also own disjoint write channels, so cross-side double-close is impossible.
The diff is largely sound. Six findings inline — one medium, the rest low. The medium (session.go Close() asymmetry) is worth addressing before merge; it leaks a goroutine in the same failure class #2285 set out to fix.
| return s.readChannel | ||
| } | ||
|
|
||
| func (s *localSession) Close() { |
There was a problem hiding this comment.
medium — Close() does not release a peer blocked in send().
Close() is asymmetric: it closes the local write channel, which releases a peer blocked in Receive(), but nothing releases a peer blocked in send(). send selects only on its own closedChan and ctx.Done(), so a peer stuck on a full writeChannel is not woken by the other side closing.
Confirmed with a probe test: fill the 10-slot buffer, block an 11th left.Send, then call right.Close() — the sender stays blocked (only left.Close() or ctx cancellation frees it). Because the closed side's Receive() now returns nil, nothing will ever drain that buffer, so the blocked sender's goroutine leaks permanently — the same failure class #2285 set out to fix.
A close signal shared by the session pair (or selecting on the peer's closedChan) would cover both directions.
| return c.cache.Get(key) | ||
| } | ||
|
|
||
| func (c *ristrettoCache[T]) Add(key string, value T) { |
There was a problem hiding this comment.
low/medium — dropping Wait() removes read-your-write for new keys.
ristretto's SetWithTTL only applies immediately via storedItems.Update when the key already exists (v2.4.2 cache.go:350); a brand-new key lives only in setBuf until the background processItems goroutine picks it up.
sherdlock's fetcher got a compensating Wait() in updateCache, but the other production consumer — tokens.Service.RequestsCache (manager.go:63) — did not, and the tokens.Cache interface has no Wait. So CacheRequest (tokens.go:190) followed shortly by getActions/GetCachedTokenRequest can now miss and fall back to a full extractActions re-parse, or to a ttxdb read plus ProcessTokenRequest (finality/listener.go:121).
Both fallbacks are correct, so this is not a correctness break — but it silently erodes the very commit-path optimization this commit is about, and the asymmetry with the fetcher looks unintentional.
| func (t *DBTransaction) FlushEvents(ctx context.Context) { | ||
| pending := t.pending | ||
| t.pending = nil | ||
| t.pending = t.pending[:0] |
There was a problem hiding this comment.
low — pending[:0] aliases the slice being iterated.
pending := t.pending; t.pending = t.pending[:0] makes the slice being iterated alias t.pending's backing array. FSC's simple.EventBus.Publish invokes subscribers synchronously (platform/view/services/events/simple/eventbus.go:30), so any subscriber that reaches back into Notify on the same DBTransaction appends over pending[0] mid-loop: the original event is lost and the newly recorded one is published in its place.
The previous t.pending = nil had no such hazard, and it only costs one allocation per transaction.
Same at storage.go:296 (Rollback), where [:0] additionally keeps the discarded events reachable from the backing array instead of dropping them.
|
|
||
| return nil | ||
| select { | ||
| case s.writeChannel <- msg: |
There was a problem hiding this comment.
low — send returns nil after the peer closed, silently dropping the message.
After the peer has closed, send still returns nil: the local session's closed flag is false and the peer's read buffer has room, so the payload is accepted into a channel whose reader (peer.Receive()) now returns nil and will never drain it.
Concretely, in startLocal (auditor.go:236) cleanupSessions closes left at the end of CollectEndorsementsView while the AuditApproveView goroutine is still running (it does Append + CacheRequest after signAndSendBack); any further right.Send reports success while the message is silently dropped, instead of surfacing "session is closed".
| return nil | ||
| case <-s.closedChan: | ||
| return errors.New("session is closed") | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
low — case <-ctx.Done() races an immediately-ready buffered send.
Go picks uniformly among ready cases, so a Send made with an already-cancelled context but free buffer space delivers ~50% of the time and returns "context cancelled while sending message" the other ~50% — nondeterministic, and a latent flake source.
Attempting a non-blocking send first (select { case ch <- msg: default: }) before falling through to the blocking select would make it deterministic.
| // DeleteTokens marks the tokens as spent in the database, attributed to the caller's stack trace. | ||
| func (t *Service) DeleteTokens(ctx context.Context, ids ...*token2.ID) (err error) { | ||
| return t.DeleteTokensBy(ctx, string(debug.Stack()), ids...) | ||
| deletedBy := "Service.DeleteTokens" |
There was a problem hiding this comment.
low — spent_by records a constant string when debug logging is off.
With debug logging off (the production default), Service.DeleteTokens now writes the literal string "Service.DeleteTokens" into the spent_by column for every token it removes. WhoDeletedTokens therefore returns a constant with no attribution value for tokens deleted via PruneInvalidUnspentTokens or the public DeleteTokens API, and the stored value differs between debug and non-debug deployments.
The commit-path attribution is unaffected — AppendValid still passes the real txID — and integration/token/fungible/tests.go:814 only asserts on that path, so no test breaks.
If the field is meant to be diagnostic, a stable caller identifier would be better than a constant.
76ac39e to
a914243
Compare
…FDT-Panurus#2285 Problem: info.Closed in localSession was read and written without synchronization, causing a data race under -race during self-auditing flows (where an issuer is also an auditor). When a session is closed (e.g. via cleanupSessions), the peer sitting in <-session.Receive() was never unblocked and waited out the full DefaultReceiveTimeout (10 seconds). Solution: - Added sync.RWMutex, closedChan, inFlight tracking, and writeClosed boolean to localSession. - Implemented Send, SendWithContext, SendError, SendErrorWithContext, Receive, and Close with proper mutex synchronization. - Close() safely closes writeChannel when inFlight == 0, releasing peers waiting on <-peer.Receive(). - Removed root-level plan.md artifact. - Added unit tests in session_test.go covering peer unblocking on close, concurrent usage under -race, and double session closure. Fixes LFDT-Panurus#2285 Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
Optimize performance and memory allocations on the transaction commit path: - Remove synchronous c.cache.Wait() calls from Ristretto cache Add/Delete/Clear - Pass TokenToAppend by pointer (*TokenToAppend) to eliminate struct copying - Retain event slice capacity across transactions (t.pending[:0]) - Preallocate slice capacities for toSpend, toAppend, and toDelete - Guard debug.Stack() and DebugfContext calls behind logger level checks Fixes LFDT-Panurus#2188 Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
…findings Address review findings on commit path optimizations and localSession: - Release peer blocked in send() on session Close() via peerClosedChan - Prevent silent message drops by rejecting send() after peer closed - Ensure deterministic non-blocking send prior to context cancellation check - Restore synchronous Wait() in Ristretto cache Add, Delete, and Clear - Reset pending events slice to nil in DBTransaction FlushEvents and Rollback - Record caller location via runtime.Caller(1) in DeleteTokens Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
a914243 to
a39395e
Compare
Summary
Optimizes performance and memory allocations on the transaction commit path in
token/services/tokensandtoken/services/utils/cache:ristretto.go): Removed synchronousc.cache.Wait()calls from write paths (Add/Delete/Clear), unlocking asynchronous Ristretto cache throughput. Exposed explicitWait()helper for testing.storage.go&tokens.go): ChangedAppendTokenparameter to pointer (*TokenToAppend) and updatedCacheEntry.ToAppend,getActions,extractActions, andParseto pass[]*TokenToAppend, eliminating 13-field struct copying (~200+ bytes per output).storage.go): Reset event buffer viat.pending = t.pending[:0]inFlushEventsandRollbackto retain allocated backing array capacity across transactions.tokens.go): Preallocated capacities fortoSpend,toAppend, andtoDeleteslices.tokens.go): Guardeddebug.Stack()inService.DeleteTokensbehindlogger.IsEnabledFor(zapcore.DebugLevel).tokens.go&storage.go): Wrapped hot-pathDebugfContextlog calls withif logger.IsEnabledFor(zapcore.DebugLevel)to eliminate Go variadic[]anyheap boxing allocations when debug logging is disabled.Fixes #2188