performance: cache the ID for Header entities

[jwinkler2083233]

No description provided.

[codecov-commenter]

Codecov Report

Merging #1279 (8c21989) into master (698c774) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1279      +/-   ##
==========================================
+ Coverage   54.96%   55.01%   +0.04%     
==========================================
  Files         501      501              
  Lines       31713    31739      +26     
==========================================
+ Hits        17432    17461      +29     
+ Misses      11925    11921       -4     
- Partials     2356     2357       +1     

Flag	Coverage Δ
unittests	55.01% <100.00%> (+0.04%)	⬆️

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

Impacted Files	Coverage Δ
model/flow/header.go	86.58% <100.00%> (+6.22%)	⬆️
...ngine/common/synchronization/finalized_snapshot.go	68.75% <0.00%> (-4.17%)	⬇️
...sus/approvals/assignment_collector_statemachine.go	47.11% <0.00%> (+4.80%)	⬆️

---

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 698c774...8c21989. Read the comment docs.

[AlexHentschel]

Thanks for the suggestion. Overall, I have strong concerns regarding the thread safety of this approach (see comments below). For merging this into master, I would need a rigorous argument why this is thread safe on all hardware platforms (based on the go memory model and potentially additional statements in the go developer forums).
Maybe I am missing something.

Furthermore, I feel we might be able to do much better, if we cache the IDs for all headers that are stored in memory, no just the one we most recently computed the ID for. There are many cases, where we traverse the chain of recent blocks, each time we receive a new one. Caching only the last header ID will not help because much, because we don't hit the same header repeatedly.

[jwinkler2083233]

Thanks for the suggestion. Overall, I have strong concerns regarding the thread safety of this approach (see comments below). For merging this into master, I would need a rigorous argument why this is thread safe on all hardware platforms (based on the go memory model and potentially additional statements in the go developer forums).
Maybe I am missing something.

Furthermore, I feel we might be able to do much better, if we cache the IDs for all headers that are stored in memory, no just the one we most recently computed the ID for. There are many cases, where we traverse the chain of recent blocks, each time we receive a new one. Caching only the last header ID will not help because much, because we don't hit the same header repeatedly.

According to the testing, we were accessing the same header repeatedly, so I'm only checking the previous one. I suspect that looking further back in history would cause a performance issue, because it's a linear search, and each compare is about a 1000-byte binary comparison.

How about this idea: what if, with each MakeID call, we also create a Murmur3 hash with the result from Rlp.encodeBytes(), and store that in a deque-based cache of limited length for ID searching ?

[bluesign]

I am go noob but I think this doesn't solve race conditions + getting makeID in mutex is a bad idea.

I think what you need is more like this:

mutex_hdr.Lock()
	prev_hdr_copy = prev_hdr
	previd_hdr_copy = previd_hdr
	h_copy = h
mutex_hdr.Unlock()

if equal(prev_hdr_copy, h_copy) 
	return previd_hdr_copy

newid = MakeID(h_copy)

mutex_hdr.Lock()
	prev_hdr = h_copy
	previd_hdr = newid
mutex_hdr.Unlock()

return newid

[jwinkler2083233]

I am go noob but I think this doesn't solve race conditions + getting makeID in mutex is a bad idea.

I think what you need is more like this:

mutex_hdr.Lock()
	prev_hdr_copy = prev_hdr
	previd_hdr_copy = previd_hdr
	h_copy = h
mutex_hdr.Unlock()

if equal(prev_hdr_copy, h_copy) 
	return previd_hdr_copy

newid = MakeID(h_copy)

mutex_hdr.Lock()
	prev_hdr = h_copy
	previd_hdr = newid
mutex_hdr.Unlock()

return newid

yes and no..

The only place I'm seeing reentrancy is during the unit tests. There's no reason to make a deep copy of h, because it is on the stack. For Header entity types, it appears from looking at the logs that the MakeID calls are not done across threads, and that they are sequential. What you're suggesting is to make a deep copy of the ID and a Header object. I can avoid the deep copy by using the mutex around the entire function.

For other entity types, like Transaction, I would agree with you. If the functions involved with MakeID for Header were more intensely concurrent, I would also agree with you.

[jwinkler2083233]

These requested changes are addressed with the most recent commit.

[bluesign]

Thanks for the answer. One more question, why not make ID as part of the Header struct and calculate on first ID() call if necessary, return same if calculated already.

It states that fingerprint is calculated on the immutable part of the header. So technically this will allow caching of all header IDs.

[AlexHentschel]

Thanks for the revisions. Some minor suggestions regarding the coding style.

Regarding the question whether to cache the ID for one or multiple headers:

• In my mind, your approach taken in this PR is very light-weight to address the most severe performance bottleneck. I think we were anyway viewing it as a temporary fix, right?

How about this idea: what if, with each MakeID call, we also create a Murmur3 hash with the result from Rlp.encodeBytes(), and store that in a deque-based cache of limited length for ID searching ?

Overall, I think your existing fix is totally fine as a temporary solution. Lets stick with that. Utilizing a cache seems to me a long-term solution

[AlexHentschel]

@bluesign

why not make ID as part of the Header struct and calculate on first ID() call if necessary, return same if calculated already.

Yes; from my perspective, this is the long-term goal. But it is a non-trivial task, because nodes are operating in a malicious environment. We can't just send the ID of a block (or header) over the wire and the recipient trusts that this value is correct. Instead, the recipient would compute the ID value when receiving the block. Essentially we would need a transformation step from "untrusted data structure" to "validated data structure". Only the validated data structure should contain a pre-computed ID. A node can locally cache the validated data structure (including the pre-computed ID).

To make the situation even more complex, it is established best practise (for blockchain systems) to only store the "untrusted data structure" in the node's data bases. When retrieving the block from the DB, the node would then re-compute the ID, which guarantees that the data has not been altered.
The motivation behind this practise is that data on disk could be corrupted and we should be validating it when retrieving.

[huitseeker]

Concept ACK.

Idle musings:
How often do we call this in contexts requiring deep equality, I wonder? Is it possible that we ferry around a header by reference far enough that we forget how to get its ID, rather than actually compare blocks from different source? In other terms, could we try pointer equality optimistically, and degrade to the full comparison when the pointers don't match?

[AlexHentschel]

@huitseeker @ramtinms I view this as a temporary solution. Hence, I am not concerned if we are not achieving max performance with this patch. I created an issue describing what I think a full solution should look like: https://github.com/dapperlabs/flow-go/issues/5853. It is going to be a bit work intensive, but hopefully not too bad 😅

[zhangchiqing]

Is this added by accident?

[zhangchiqing]

I don't quite understand this change. Wouldn't this lock block two different threads computing IDs of two headers?

If engine A is calling header1.ID(), and engine B is calling header2.ID(), before this change, they can run concurrently, but with this change, one engine has to be blocked and wait until the other finishes its call. no?