chore(deps): `timed-map` migration #2247

shamardy · 2024-10-21T10:07:04Z

Complete timed-map migration from TimeCache, DuplicateCache, ExpirableMap and ExpirableEntry.

mm2src/coins/Cargo.toml

Signed-off-by: onur-ozkan <[email protected]>

onur-ozkan · 2025-01-09T14:43:21Z

mm2src/coins/utxo/rpc_clients/electrum_rpc/connection.rs

@@ -177,7 +177,7 @@ impl ElectrumConnection {
            settings,
            tx: Mutex::new(None),
            establishing_connection: AsyncMutex::new(()),
-            responses: Mutex::new(JsonRpcPendingRequests::new()),
+            responses: Mutex::new(JsonRpcPendingRequests::new_with_map_kind(MapKind::BTreeMap)),


Picked BTreeMap intentionally to process entries in order.

wdym by processing entries in order? what processing?

wdym by processing entries in order? what processing?

Making sure to process responses in the FIFO way.

yeah where is that 😂

Fell into this terrible naming (there is no separation at all at the first glance)

komodo-defi-framework/mm2src/coins/utxo/rpc_clients/electrum_rpc/connection.rs

Lines 340 to 344 in 405bcb7

for response in responses {

// `split` returns empty slice if it ends with separator which is our case.

if !response.is_empty() {

self.process_electrum_response(response, event_handlers)

}

thought this was the loop iterating over the responses map..

Honestly I still believe we should prefer BTreeMap here to avoid difficult-to-debug magical future bugs. If we add any logic with the expectation of running FIFO processing, it will accidentally run in a random order and that will be super annoying to catch on runtime. The fact that I already falled in this trap makes it very likely to happen again to me or anyone in the team.

aha cool, im fine with BTree or others, i only nitted to make sure im not missing something with the in-order comment.

Signed-off-by: onur-ozkan <[email protected]>

mariocynicys

Thanks!
One comment inline. LGTM otherwise.

mm2src/mm2_main/src/lp_swap/swap_watcher.rs

shamardy · 2025-01-10T12:15:48Z

lp_ordermatch::ordermatch_tests::test_orderbook_order_pairs_trie_state_history_updates_expiration_on_insert and lp_ordermatch::ordermatch_tests::test_orderbook_sync_trie_diff_time_cache are both failing after the recent changes

mariocynicys · 2025-01-10T12:49:09Z

mm2src/coins/utxo/rpc_clients/electrum_rpc/connection.rs

@@ -251,7 +251,7 @@ impl ElectrumConnection {
        self.responses
            .lock()
            .unwrap()
-            .insert(rpc_id, req_tx, Duration::from_secs_f64(timeout));
+            .insert_expirable_unchecked(rpc_id, req_tx, Duration::from_secs_f64(timeout));


the only insert being unchecked basically makes this map susceptible to memory leaks. this acts pretty much similar (with an *) to a normal hashmap at this point.

you can instead make it a checked insert but use a high tick cap.

there are other never-checked maps also introduced in this PR.

the only insert being unchecked basically makes this map susceptible to memory leaks. this acts pretty much similar (with an *) to a normal hashmap at this point.

you can instead make it a checked insert but use a high tick cap.

We don't need to include expiration overhead on inserts on electrum requests. The check is performed at end of process_electrum_response which should be enough.

there are other never-checked maps also introduced in this PR.

Can you mark them? I tried to minimize the expiration overhead in various places where it's unnecessary.

We don't need to include expiration overhead on inserts on electrum requests. The check is performed at end of process_electrum_response which should be enough.

i can't find no check for outdated entries in process_electrum_response!
you aren't talking about the .remove right? .remove will not help with requests that we got no responses for, these request handles will reside in the timed map forever (until the connection terminates*).

mariocynicys

labeled the never checked maps below

mariocynicys · 2025-01-13T10:10:12Z

mm2src/coins/eth/web3_transport/websocket_transport.rs

    ) -> OuterAction {
        match request {
            Some(ControllerMessage::Request(WsRequest {
                request_id,
                serialized_request,
                response_notifier,
            })) => {
-                response_notifiers.insert(
+                response_notifiers.insert_expirable_unchecked(


mariocynicys · 2025-01-13T10:10:28Z

mm2src/coins/utxo/rpc_clients/electrum_rpc/connection.rs

@@ -251,7 +251,7 @@ impl ElectrumConnection {
        self.responses
            .lock()
            .unwrap()
-            .insert(rpc_id, req_tx, Duration::from_secs_f64(timeout));
+            .insert_expirable_unchecked(rpc_id, req_tx, Duration::from_secs_f64(timeout));


mariocynicys · 2025-01-13T10:16:39Z

mm2src/mm2_main/src/lp_ordermatch.rs

+            let history = match pubkey_state.order_pairs_trie_state_history.get_mut(&alb_ordered) {
+                Some(t) => t,
+                None => {
+                    pubkey_state.order_pairs_trie_state_history.insert_expirable_unchecked(


onur-ozkan · 2025-01-13T18:52:03Z

lp_ordermatch::ordermatch_tests::test_orderbook_order_pairs_trie_state_history_updates_expiration_on_insert and lp_ordermatch::ordermatch_tests::test_orderbook_sync_trie_diff_time_cache are both failing after the recent changes

The order trie logic highly depends on timecache internals, which is covered in the test_orderbook_order_pairs_trie_state_history_updates_expiration_on_insert test.

We have 3 options:

Change the order trie expiration logic (timecache has quite different expiration logic (e.g., bulk mode expirations) and removes expired entries on remove and entry functions, not on inserts. Also, reading functions do not consider expirations.)
Change the expectations on test_orderbook_order_pairs_trie_state_history_updates_expiration_on_insert flow.
Stay with timecache for order trie (this requires to keep time_cache module with ExpirableEntry in the expirable_map module)

I am ok with any of them.

Signed-off-by: onur-ozkan <[email protected]>

shamardy added the in progress label Oct 21, 2024

shamardy changed the title ~~chore(deps): use timed-map crate instead of internal ExpirableMap type~~ chore(deps): use timed-map crate instead of internal ExpirableMap Oct 21, 2024

onur-ozkan reviewed Nov 6, 2024

View reviewed changes

mm2src/coins/Cargo.toml Outdated Show resolved Hide resolved

onur-ozkan added status: in progress and removed in progress labels Dec 16, 2024

migrate to timed-map entirely

ff4038d

Signed-off-by: onur-ozkan <[email protected]>

onur-ozkan force-pushed the use-timed-map-crate branch from e2b814e to ff4038d Compare January 9, 2025 14:37

onur-ozkan added status: pending review improvement: dependencies and removed status: in progress labels Jan 9, 2025

onur-ozkan marked this pull request as ready for review January 9, 2025 14:41

onur-ozkan changed the title ~~chore(deps): use timed-map crate instead of internal ExpirableMap~~ chore(deps): timed-map migration Jan 9, 2025

onur-ozkan reviewed Jan 9, 2025

View reviewed changes

set expiration tick to 25 for ordermatching maps

b698473

Signed-off-by: onur-ozkan <[email protected]>

mariocynicys reviewed Jan 9, 2025

View reviewed changes

mm2src/mm2_main/src/lp_swap/swap_watcher.rs Show resolved Hide resolved

mariocynicys reviewed Jan 10, 2025

View reviewed changes

mariocynicys reviewed Jan 13, 2025

View reviewed changes

onur-ozkan added status: in progress and removed status: pending review labels Jan 13, 2025

save dev state

5563e9d

Signed-off-by: onur-ozkan <[email protected]>

onur-ozkan force-pushed the use-timed-map-crate branch from 5094612 to 5563e9d Compare January 13, 2025 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(deps): `timed-map` migration #2247

chore(deps): `timed-map` migration #2247

shamardy commented Oct 21, 2024 •

edited by onur-ozkan

Loading

onur-ozkan Jan 9, 2025

mariocynicys Jan 9, 2025

onur-ozkan Jan 13, 2025

mariocynicys Jan 13, 2025

onur-ozkan Jan 13, 2025

onur-ozkan Jan 13, 2025

mariocynicys Jan 13, 2025

mariocynicys left a comment

shamardy commented Jan 10, 2025

mariocynicys Jan 10, 2025 •

edited

Loading

onur-ozkan Jan 13, 2025

mariocynicys Jan 13, 2025

mariocynicys left a comment

mariocynicys Jan 13, 2025

mariocynicys Jan 13, 2025

mariocynicys Jan 13, 2025

onur-ozkan commented Jan 13, 2025 •

edited

Loading

	for response in responses {
	// `split` returns empty slice if it ends with separator which is our case.
	if !response.is_empty() {
	self.process_electrum_response(response, event_handlers)
	}

chore(deps): timed-map migration #2247

Are you sure you want to change the base?

chore(deps): timed-map migration #2247

Conversation

shamardy commented Oct 21, 2024 • edited by onur-ozkan Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariocynicys left a comment

Choose a reason for hiding this comment

shamardy commented Jan 10, 2025

mariocynicys Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariocynicys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan commented Jan 13, 2025 • edited Loading

chore(deps): `timed-map` migration #2247

chore(deps): `timed-map` migration #2247

shamardy commented Oct 21, 2024 •

edited by onur-ozkan

Loading

mariocynicys Jan 10, 2025 •

edited

Loading

onur-ozkan commented Jan 13, 2025 •

edited

Loading