hotstuff: improve the latency of hotstuff verification

[jwinkler2083233]

This will remove the requirement for a mutex, and just use
atomic instructions instead.

[codecov-commenter]

Codecov Report

Merging #1523 (db03671) into master (38957a7) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1523      +/-   ##
==========================================
- Coverage   55.33%   55.21%   -0.12%     
==========================================
  Files         527      535       +8     
  Lines       32626    33118     +492     
==========================================
+ Hits        18053    18287     +234     
- Misses      12161    12393     +232     
- Partials     2412     2438      +26     

Flag	Coverage Δ
unittests	55.21% <100.00%> (-0.12%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
consensus/hotstuff/verification/common.go	85.45% <100.00%> (+0.26%)	⬆️
model/flow/seal.go	60.00% <0.00%> (-40.00%)	⬇️
admin/server.go	80.00% <0.00%> (-20.00%)	⬇️
model/flow/execution_result.go	25.80% <0.00%> (-8.98%)	⬇️
model/flow/execution_receipt.go	52.72% <0.00%> (-8.98%)	⬇️
network/test/testUtil.go	88.09% <0.00%> (-5.58%)	⬇️
ledger/common/hash/hash.go	40.00% <0.00%> (-4.45%)	⬇️
model/flow/ledger.go	8.33% <0.00%> (-4.17%)	⬇️
network/p2p/dns/resolver.go	97.53% <0.00%> (-2.47%)	⬇️
model/flow/identifier.go	71.21% <0.00%> (-1.26%)	⬇️
... and 42 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38957a7...db03671. Read the comment docs.

[bluesign]

I think having inner here later can cause some complications. ( confusing )

[jwinkler2083233]

Maybe a name change or comment would help? What would you like to see ?

[bluesign]

I think updateCurrent would be better choice here, without inner

[jwinkler2083233]

I think I tried that and there was a problem in the tests. If you want to paste your code change suggestion, I'll try it.

[bluesign]

I think if this mutex is causing performance problems, it is just a symptom not the cause of the problem.

But I really liked the update, avoiding mutex like this is also very cool.

[AlexHentschel]

Took a brief pass over the PR and it looks great. However concurrency safety probably warrants some detailed comments in the code is:

• note that according to the Go memory model, atomic reads and writes do not establish a happens-before relationship
  • we have discussed this in more detail in slack thread
  • Most relevant reference is this one

    Go's atomics guarantee sequential consistency among the atomic variables (behave like C/C++'s seqconst atomics). You shouldn't mix atomic and non-atomic accesses for a given memory word, unless some other full memory barrier, like a Mutex, guarantees exclusive access. You shouldn't mix atomics of different memory sizes for the same address.

• long story short, synchronization guarantees in Go for atomic variables and subsequent non-atomic memory writes are far from clearly specified.

In your particular implementation, a reviewer might ask: "There is no concurrency protection to updatedSignerSet and go's atomics don't establish a happens-before relationship. How is concurrency safety guaranteed here?" (previously we had a mutex, which establishes a happens-before relationship and thereby guarantees concurrency safety).
From my perspective, the answer is that aggregate and all data fields therein (including updatedSignerSet and updatedKey) are essentially immutable. A new data structure is allocated for each write. Therefore, it is sufficient to guarantee sequential consistency by using an atomic variable. But we don't have to take any additional measures to mediate mutations of aggregate as such mutations don't happen within the implementation.

Hope that makes sense (?)

[jwinkler2083233]

I'm closing this for now, because tests are inconclusive