BTreeMap for indices_to_hash_map

[skaunov]

This stems from #543 but focuses on the different aspect. It would not be much reasonable to change a function because of a test, but I looked into itself and that seems it only benefits from switching, as this improves find. (The proptest benefits by the determinism of BTreeMap.)

[aszepieniec]

Can you be more specific about what is improved? In general, accesses in a binary tree map take $O(\log n)$ whereas accesses in a hash map take $O(1)$ time, so I'd like to understand the apparent performance improvement properly before approving this PR.

[skaunov]

@aszepieniec
My intuition was that both partition and find operations in split_by_activity do in essence an u64 comparison so finding the correct root in a tree or checking the right most leaf should be easier than going over all the .keys() arbitrary. But that's just intuition of mine; I didn't dig for an evidence. 🙊

[aszepieniec]

Let's collect evidence so that we do not have to rely on intuition to determine whether merging this PR is a good idea.

Or we can file this PR under "too low priority for now" and shelve it until we need it.

[skaunov]

Sounds good to me. I can redo it with `.last()` instead of `.find` and a range set instead of `partition` to make retrieval benefit obvious. Indicate if you would want to merge it then, pls. As for inserting comparison the whole issue feels too small to actually analyze and compare. I even suspect that with such small lengths of the set the comparison won't be definite and mostly reflect other things like the platform, `rustc` version, hardware, and such.

…

On Вс, апр 20 2025 at 01:04:50 -0700, aszepieniec ***@***.***> wrote: Let's collect evidence so that we do not have to rely on intuition to determine whether merging this PR is a good idea. Or we can file this PR under "too low priority for now" and shelve it until we need it. — Reply to this email directly, view it on GitHub <#554 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APXLOT5ETX3XCLJPOWFNLJD22NIKFAVCNFSM6AAAAAB3KDGVWKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMJXGA2TENJUGY>. You are receiving this because you authored the thread.Message ID: ***@***.***> *aszepieniec* left a comment (Neptune-Crypto/neptune-core#554) <#554 (comment)> Let's collect evidence so that we do not have to rely on intuition to determine whether merging this PR is a good idea. Or we can file this PR under "too low priority for now" and shelve it until we need it. — Reply to this email directly, view it on GitHub <#554 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APXLOT5ETX3XCLJPOWFNLJD22NIKFAVCNFSM6AAAAAB3KDGVWKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMJXGA2TENJUGY>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[aszepieniec]

Sounds good to me. I can redo it with .last() instead of .find and
a range set instead of partition to make retrieval benefit obvious.
Indicate if you would want to merge it then, pls.

I don't know, it's not obvious before I can see the code. I can only pass judgment afterwards.

I'm happy shelving this discussion until it is clear we need this particular thing to be faster. I have a hunch that other things are much more deserving of our attention when it comes to optimization.

[aszepieniec]

Closed: shelved indefinitely.