tx_pool: update internal data structure to boost::bimap. #9376
Conversation
startup speed up 2-5x because this PR changing O(n^2) operation to O(nlogn).
|
(plows here) are external factors required to test this? (e.g. a huge mempool) |
No external thing necessary. Go ahead and test. and please let me know if you have any feedback. Thanks. |
|
Regarding the failure in PR, it is race failure we have, unrelated to this PR. |
| } | ||
| }; | ||
|
|
||
| //! container for sorting transactions by fee per unit size | ||
| typedef std::set<tx_by_fee_and_receive_time_entry, txCompare> sorted_tx_container; | ||
| typedef boost::bimap<boost::bimaps::multiset_of<std::pair<double, std::time_t>, txFeeCompare>, | ||
| boost::bimaps::set_of<crypto::hash, hashCompare>> sorted_tx_container; |
There was a problem hiding this comment.
What about using unordered_set_of here? This could accelerate the lookup, but would likely require more memory (for the table), and perhaps resizing the table would be slow.
There was a problem hiding this comment.
What do you think? At this point it comes to the design choices, I am perfectly happy to change it to unordered_set_of.
There was a problem hiding this comment.
Search times are a bit lower with an unordered_map, but insertion times may be slower with a large number of keys. I think we have more searches than insertions, but I am not certain. So just leave it for now, unless. you want to run another test.
There was a problem hiding this comment.
Thanks a lot for detailed explanation.
| @@ -751,13 +753,9 @@ namespace cryptonote | |||
| m_remove_stuck_tx_interval.do_call([this](){return remove_stuck_transactions();}); | |||
| } | |||
| //--------------------------------------------------------------------------------- | |||
| sorted_tx_container::iterator tx_memory_pool::find_tx_in_sorted_container(const crypto::hash& id) const | |||
| sorted_tx_container::iterator tx_memory_pool::find_tx_in_sorted_container(const crypto::hash& id) | |||
There was a problem hiding this comment.
I looked at the documentation, and it looks like this function can remain const? What broke this?
There was a problem hiding this comment.
I just noticed that iterator has to be returned, so presumably the reason is find and project_up return const_iterator here.
There was a problem hiding this comment.
Yes you are right. If it remains const then the find will return const_iterator and that is causing a lot headache. We want to use that iterator in erase which doesn't accept const_iterator, and no easy way of turning const_iterator to iterator in boost.
0xFFFC0000
changed the title
|
@vtnerd could you approve if all your questions are answered? |
startup speed up 2-5x because this PR changing
O(n^2)operation toO(nlogn).Almost all people involved in stressnet experiment who tested this patch, reported at least 2.5x speed up of startup time of monerod.
Thanks to @Boog900, @spackle-xmr, Rucknium and ofrnxmr for their help