Avoid using ordered maps in LLMQ signing code#2708
Conversation
|
I mean I reviewed parts that are specific to this PR and they look good to me (LGTM) but I will formally review/ack this after rebase :) |
|
Ahhh, so "LGTM" doesn't mean "Let's get this merged!" :D 🙈 Learned something new (it's "Looks good to me!") :) |
3439377 to
fc2367c
Compare
|
Rebased and force-pushed |
UdjinM6
left a comment
There was a problem hiding this comment.
utACK
I wonder if you have some benchmarks for "old code vs new code" 👀
|
needs rebase |
The old implementation was relying on the maps being ordered, which allowed us to grab all sig shares for the same signHash by doing range queries on the map. This has the disadvantage of being unnecessarily slow when the maps get larger. Using an unordered map would be the naive solution, but then it's not possible to query by range anymore. The solution now is to have a specialized map "SigShareMap" which is indexed by "SigShareKey". It's internally just an unordered map, indexed by the sign hash and another unordered map for the value, indexed by the quorum member index.
These are faster when maps/sets get larger.
fc2367c to
81bd958
Compare
|
Rebased and force-pushed. @UdjinM6 regarding benchmarks: I didn't do benchmarks in the usual way, as this code is too complex to isolate individual parts to benchmark. I did the following flow multiple times:
The HotSpot tool shows so called "flame graphs", which give some very good hints on where most time is spent. It can filter per thread, you can zoom in, and so on. The For these tests, the data usually shows (as expected) 2 threads which do the most work. The first is the message handler thread and the second one is the My target was to reduce the amount of time spent for LLMQ messages in the message handler thread and move all the load to the other thread. The reason is, that processing one message from one peer might end up blocking receival/processing of many other messages from all other peers. Many optimizations in the LLMQ code assume that many signatures come in at once and can be batch verified in one go. If the message handler thread needs too much time per message, the work thread will end up verifying batches of size one too often. This is ok when the load is low, but gets worse and worse as higher the load becomes. At the same time, I tried to reduce the overhead seen in the work thread. The assumption is that most time should be spent on signature (batch) verification and a little bit on signing. Everything else is overhead and should be reduced as much as possible. This is for example all the time spent in book keeping of the sig shares, which IMHO should be minimal. The first rounds of the profiling have shown that actually a lot of time (~10%) was spent in std::map::find in various places. The first rounds have also shown that even more time (don't remember the percentage) was spent in LevelDB due to many calls to The first rounds of profiling have also shown that After all the optimizations, I'm pretty happy with how the flame graphs look now. Most of the time is spent in signature verification and signing, which is what I would expect. When everything is merged (including @UdjinM6's changes to intra quorum comms), I'll do another round of profiling and post some screenshots of the flame graphs. |
|
Ohhh, nice :) 👍 |




As profiling has shown, these get too expensive when they get larger. Better to use unordered maps whenever possible.
This PR currently also includes #2706 as it depends on the hashing forstd::pair<Consensus::LLMQType, uint256>. I'll remove it when it got merged.