feat(collator): refactor message groups #316

@@ -1273,6 +1279,10 @@ impl CollatorStdImpl {
            drop(histogram);
            self.do_collate(working_state, None).await?;
        } else {
+            if !has_uprocessed_messages && !has_externals {


has_unprocessed_messages

0xdeafbeef · 2024-10-02T08:47:57Z

collator/src/collator/message_group_v2.rs

+    pub fn extract_merged_group(&mut self) -> Option<MessageGroup> {
+        let mut messages: Vec<(HashBytes, VecDeque<Message>)> =
+            self.current_messages.drain().collect();
+
+        while let Some((key, val)) = self.new_messages_full.pop_first() {
+            messages.push((key, val));
+        }
+
+        while let Some((key, val)) = self.new_messages.pop_first() {
+            messages.push((key, val));
+        }
+
+        if messages.is_empty() {
+            return None;
+        }
+
+        let group = MessageGroup::new(
+            messages
+                .into_iter()
+                .map(|(h, m)| (h, m.into_iter().map(|m| m.inner).collect()))
+                .collect(),
+            self.int_messages_count,
+            self.ext_messages_count,
+        );
+
+        tracing::debug!(target: tracing_targets::COLLATOR,
+            "extracted merged message group of new messages from message_groups buffer: buffer int={}, ext={}, group {}",
+            self.int_messages_count(), self.ext_messages_count(),
+            DisplayMessageGroup(&group),
+        );
+
+        Some(group)
+    }
+}


Shouldn't we update counters in self after moving messages into a new MessageGroup?

0xdeafbeef · 2024-10-02T08:52:22Z

collator/src/collator/message_group_v2.rs

+    new_messages: BTreeMap<HashBytes, VecDeque<Message>>,
+    new_messages_full: BTreeMap<HashBytes, VecDeque<Message>>,


Can't it be changed to IndexMap ?

0xdeafbeef · 2024-10-02T08:53:42Z

collator/src/collator/mod.rs

@@ -204,6 +209,7 @@ pub struct CollatorStdImpl {
    anchors_cache: AnchorsCache,
    stats: CollatorStats,
    timer: std::time::Instant,
+    accounts_load: BTreeMap<AccountId, u32>,


Can't this be changed to HashMap? It seems like order doesn't matter

codecov · 2024-10-09T15:30:12Z

Codecov Report

Attention: Patch coverage is 70.56962% with 93 lines in your changes missing coverage. Please review.

Project coverage is 47.13%. Comparing base (21577f8) to head (f9f488c).

Files with missing lines	Patch %	Lines
collator/src/collator/message_group_v1.rs	75.86%	49 Missing and 7 partials ⚠️
collator/src/collator/types.rs	56.62%	32 Missing and 4 partials ⚠️
collator/src/collator/mod.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
+ Coverage   46.99%   47.13%   +0.13%     
==========================================
  Files         240      241       +1     
  Lines       37403    37530     +127     
  Branches    37403    37530     +127     
==========================================
+ Hits        17578    17688     +110     
- Misses      18909    18924      +15     
- Partials      916      918       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

serejkaaa512 force-pushed the feature/optimize-messages-groups branch 6 times, most recently from d77124a to e318e94 Compare September 20, 2024 06:21

SmaGMan requested changes Sep 20, 2024

View reviewed changes

collator/src/collator/execution_manager.rs Outdated Show resolved Hide resolved