-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tx_pool: update internal data structure to boost::bimap. #9376
base: master
Are you sure you want to change the base?
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just a few questions.
} | ||
}; | ||
|
||
//! 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the documentation, and it looks like this function can remain const
? What broke this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@vtnerd could you approve if all your questions are answered? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok to me.
In particular, as the code is already shown as working, I concentrated on an attempt to find possible unfortunate side effects of the change of storing that info, and found none: Things are simple enough.
My only comment is a proposal to delete a typedef
that is probably not needed anymore - nothing to prevent me from approving already.
@@ -62,24 +65,33 @@ namespace cryptonote | |||
//! pair of <transaction fee, transaction hash> for organization | |||
typedef std::pair<std::pair<double, std::time_t>, crypto::hash> tx_by_fee_and_receive_time_entry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure that typedef is not needed anymore and can be deleted, in the interest of clean code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comment. Yes. it was not needed. But didn't want to make any unnecessary changes.
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