Fix comm stack resource consumption #1034
Conversation
mbrandenburger
force-pushed
the
comm-perf-fixes
branch
from
ccb3ad4 to
1a0bb38
Compare
|
This screenshot shows the memory consumption (memory leak) and exploding number of goroutines with and without the fix in this PR. 
|
|
@mbrandenburger , wow, super interesting. Only the rate of objects allocated got worse. Do I read it correctly? |
No no, not worse. It increased was the FSC node was not able to process at a higher rate. Here is a screen shot of TPS/latency for the view invocation. You can see, with the fix in this PR, the TPS ~doubled and the latency remains stable. Thus, this the node is processing more invocations, the object allocation rate is highter. 
|
|
I am running this fix now with the TokenSDK hyperledger-labs/fabric-token-sdk#1217 🤞 |
mbrandenburger
requested review from
adecaro,
ale-linux,
alexandrosfilios and
arner
and removed request for
alexandrosfilios
mbrandenburger
force-pushed
the
comm-perf-fixes
branch
from
1a0bb38 to
24a9545
Compare
mbrandenburger
changed the title
ale-linux
left a comment
ale-linux
left a comment
There was a problem hiding this comment.
Awesome stuff @mbrandenburger! Only issue I see is that getting rid of the debug printouts might bite is in the ass later... how about we leave them encased in an if that checks whether the debug level is enabled?
| _ = s.Close() | ||
| return | ||
| } | ||
| logger.Debugf("Read message of length [%d] on [%s]", len(msg), s.Hash()) |
There was a problem hiding this comment.
Might it not be useful to have such a debug log?
There was a problem hiding this comment.
if it is we could put it within a logger.IsEnabledFor(zapcore.DebugLevel) if statement.
There was a problem hiding this comment.
good catch! I restored the log here and put it inside the if clause using logger.IsEnabledFor(zapcore.DebugLevel). Thanks guys.
arner
left a comment
arner
left a comment
There was a problem hiding this comment.
Nice, looks like a big improvement!
| _ = s.Close() | ||
| return | ||
| } | ||
| logger.Debugf("Read message of length [%d] on [%s]", len(msg), s.Hash()) |
There was a problem hiding this comment.
if it is we could put it within a logger.IsEnabledFor(zapcore.DebugLevel) if statement.
|
|
||
| output := make([]*comm.ViewPacket, 0, len(input)) | ||
| m := sync.RWMutex{} | ||
| go func() { |
There was a problem hiding this comment.
maybe I'm missing something but does this have to be in a goroutine + waitgroup or can it just as well be inlined?
There was a problem hiding this comment.
given that the read and write channels are buffered, we could indeed first write all messages to the stream and then read without an spawning the goroutine to do that. I don't have strong preferences here.
- set reasonable read buffer size - cleanup subcon when they are closed - more efficient sessionID computation - remove delayed subcon close - remove delayed context deletion after view execution Signed-off-by: Marcus Brandenburger <bur@zurich.ibm.com>
mbrandenburger
force-pushed
the
comm-perf-fixes
branch
from
24a9545 to
ecd7af1
Compare
4 participants
The current stack suffers from high resource utilization (active number of goroutines and increasing memory usage) when invoking views through the p2p comm layer.
This PR tames the resource consumption.