Skip to content

Commit c5f2460

Browse files
committed
fix(sdk): Disable OrderTracker for the moment.
This patch removes the use of `OrderTracker` because the implementation of `EventCacheStore::load_all_chunks_metadata` for `SqliteEventCacheStore` is the cause of severe slownesses (up to 100s for some account). We are going to undo this patch once the problem has been solved.
1 parent 8ad785a commit c5f2460

File tree

3 files changed

+30
-585
lines changed

3 files changed

+30
-585
lines changed

crates/matrix-sdk/src/event_cache/room/events.rs

Lines changed: 11 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ use matrix_sdk_base::{
1919
event_cache::store::DEFAULT_CHUNK_CAPACITY,
2020
linked_chunk::{
2121
lazy_loader::{self, LazyLoaderError},
22-
ChunkContent, ChunkIdentifierGenerator, ChunkMetadata, OrderTracker, RawChunk,
22+
ChunkContent, ChunkIdentifierGenerator, RawChunk,
2323
},
2424
};
2525
use matrix_sdk_common::linked_chunk::{
@@ -38,9 +38,6 @@ pub(in crate::event_cache) struct EventLinkedChunk {
3838
///
3939
/// [`Update`]: matrix_sdk_base::linked_chunk::Update
4040
chunks_updates_as_vectordiffs: AsVector<Event, Gap>,
41-
42-
/// Tracker of the events ordering in this room.
43-
pub order_tracker: OrderTracker<Event, Gap>,
4441
}
4542

4643
impl Default for EventLinkedChunk {
@@ -52,27 +49,22 @@ impl Default for EventLinkedChunk {
5249
impl EventLinkedChunk {
5350
/// Build a new [`EventLinkedChunk`] struct with zero events.
5451
pub fn new() -> Self {
55-
Self::with_initial_linked_chunk(None, None)
52+
Self::with_initial_linked_chunk(None)
5653
}
5754

5855
/// Build a new [`EventLinkedChunk`] struct with prior chunks knowledge.
5956
///
6057
/// The provided [`LinkedChunk`] must have been built with update history.
6158
pub fn with_initial_linked_chunk(
6259
linked_chunk: Option<LinkedChunk<DEFAULT_CHUNK_CAPACITY, Event, Gap>>,
63-
full_linked_chunk_metadata: Option<Vec<ChunkMetadata>>,
6460
) -> Self {
6561
let mut linked_chunk = linked_chunk.unwrap_or_else(LinkedChunk::new_with_update_history);
6662

6763
let chunks_updates_as_vectordiffs = linked_chunk
6864
.as_vector()
6965
.expect("`LinkedChunk` must have been built with `new_with_update_history`");
7066

71-
let order_tracker = linked_chunk
72-
.order_tracker(full_linked_chunk_metadata)
73-
.expect("`LinkedChunk` must have been built with `new_with_update_history`");
74-
75-
Self { chunks: linked_chunk, chunks_updates_as_vectordiffs, order_tracker }
67+
Self { chunks: linked_chunk, chunks_updates_as_vectordiffs }
7668
}
7769

7870
/// Clear all events.
@@ -194,36 +186,6 @@ impl EventLinkedChunk {
194186
self.chunks.items()
195187
}
196188

197-
/// Return the order of an event in the room linked chunk.
198-
///
199-
/// Can return `None` if the event can't be found in the linked chunk.
200-
pub fn event_order(&self, event_pos: Position) -> Option<usize> {
201-
self.order_tracker.ordering(event_pos)
202-
}
203-
204-
#[cfg(any(test, debug_assertions))]
205-
#[allow(dead_code)] // Temporarily, until we figure out why it's crashing production builds.
206-
fn assert_event_ordering(&self) {
207-
let mut iter = self.chunks.items().enumerate();
208-
let Some((i, (first_event_pos, _))) = iter.next() else {
209-
return;
210-
};
211-
212-
// Sanity check.
213-
assert_eq!(i, 0);
214-
215-
// That's the offset in the full linked chunk. Will be 0 if the linked chunk is
216-
// entirely loaded, may be non-zero otherwise.
217-
let offset =
218-
self.event_order(first_event_pos).expect("first event's ordering must be known");
219-
220-
for (i, (next_pos, _)) in iter {
221-
let next_index =
222-
self.event_order(next_pos).expect("next event's ordering must be known");
223-
assert_eq!(offset + i, next_index, "event ordering must be continuous");
224-
}
225-
}
226-
227189
/// Get all updates from the room events as [`VectorDiff`].
228190
///
229191
/// Be careful that each `VectorDiff` is returned only once!
@@ -232,11 +194,7 @@ impl EventLinkedChunk {
232194
///
233195
/// [`Update`]: matrix_sdk_base::linked_chunk::Update
234196
pub fn updates_as_vector_diffs(&mut self) -> Vec<VectorDiff<Event>> {
235-
let updates = self.chunks_updates_as_vectordiffs.take();
236-
237-
self.order_tracker.flush_updates(false);
238-
239-
updates
197+
self.chunks_updates_as_vectordiffs.take()
240198
}
241199

242200
/// Get a mutable reference to the [`LinkedChunk`] updates, aka
@@ -256,8 +214,7 @@ impl EventLinkedChunk {
256214
let mut result = Vec::new();
257215

258216
for chunk in self.chunks.chunks() {
259-
let content =
260-
chunk_debug_string(chunk.identifier(), chunk.content(), &self.order_tracker);
217+
let content = chunk_debug_string(chunk.content());
261218
let lazy_previous = if let Some(cid) = chunk.lazy_previous() {
262219
format!(" (lazy previous = {})", cid.index())
263220
} else {
@@ -400,28 +357,6 @@ impl EventLinkedChunk {
400357

401358
// Methods related to lazy-loading.
402359
impl EventLinkedChunk {
403-
/// Inhibits all the linked chunk updates caused by the function `f` on the
404-
/// ordering tracker.
405-
///
406-
/// Updates to the linked chunk that happen because of lazy loading must not
407-
/// be taken into account by the order tracker, otherwise the
408-
/// fully-loaded state (tracked by the order tracker) wouldn't match
409-
/// reality anymore. This provides a facility to help applying such
410-
/// updates.
411-
fn inhibit_updates_to_ordering_tracker<F: FnOnce(&mut Self) -> R, R>(&mut self, f: F) -> R {
412-
// Start by flushing previous pending updates to the chunk ordering, if any.
413-
self.order_tracker.flush_updates(false);
414-
415-
// Call the function.
416-
let r = f(self);
417-
418-
// Now, flush other pending updates which have been caused by the function, and
419-
// ignore them.
420-
self.order_tracker.flush_updates(true);
421-
422-
r
423-
}
424-
425360
/// Replace the events with the given last chunk of events and generator.
426361
///
427362
/// Happens only during lazy loading.
@@ -433,51 +368,33 @@ impl EventLinkedChunk {
433368
last_chunk: Option<RawChunk<Event, Gap>>,
434369
chunk_identifier_generator: ChunkIdentifierGenerator,
435370
) -> Result<(), LazyLoaderError> {
436-
// Since `replace_with` is used only to unload some chunks, we don't want it to
437-
// affect the chunk ordering.
438-
self.inhibit_updates_to_ordering_tracker(move |this| {
439-
lazy_loader::replace_with(&mut this.chunks, last_chunk, chunk_identifier_generator)
440-
})
371+
lazy_loader::replace_with(&mut self.chunks, last_chunk, chunk_identifier_generator)
441372
}
442373

443374
/// Prepends a lazily-loaded chunk at the beginning of the linked chunk.
444375
pub(super) fn insert_new_chunk_as_first(
445376
&mut self,
446377
raw_new_first_chunk: RawChunk<Event, Gap>,
447378
) -> Result<(), LazyLoaderError> {
448-
// This is only used when reinserting a chunk that was in persisted storage, so
449-
// we don't need to touch the chunk ordering for this.
450-
self.inhibit_updates_to_ordering_tracker(move |this| {
451-
lazy_loader::insert_new_first_chunk(&mut this.chunks, raw_new_first_chunk)
452-
})
379+
lazy_loader::insert_new_first_chunk(&mut self.chunks, raw_new_first_chunk)
453380
}
454381
}
455382

456383
/// Create a debug string for a [`ChunkContent`] for an event/gap pair.
457-
fn chunk_debug_string(
458-
chunk_id: ChunkIdentifier,
459-
content: &ChunkContent<Event, Gap>,
460-
order_tracker: &OrderTracker<Event, Gap>,
461-
) -> String {
384+
fn chunk_debug_string(content: &ChunkContent<Event, Gap>) -> String {
462385
match content {
463386
ChunkContent::Gap(Gap { prev_token }) => {
464387
format!("gap['{prev_token}']")
465388
}
466389
ChunkContent::Items(vec) => {
467390
let items = vec
468391
.iter()
469-
.enumerate()
470-
.map(|(i, event)| {
392+
.map(|event| {
471393
event.event_id().map_or_else(
472394
|| "<no event id>".to_owned(),
473395
|id| {
474-
let pos = Position::new(chunk_id, i);
475-
let order = format!("#{}: ", order_tracker.ordering(pos).unwrap());
476-
477396
// Limit event ids to 8 chars *after* the $.
478-
let event_id = id.as_str().chars().take(1 + 8).collect::<String>();
479-
480-
format!("{order}{event_id}")
397+
id.as_str().chars().take(1 + 8).collect::<String>()
481398
},
482399
)
483400
})
@@ -762,13 +679,10 @@ mod tests {
762679
]);
763680
linked_chunk.chunks.push_gap_back(Gap { prev_token: "raclette".to_owned() });
764681

765-
// Flush updates to the order tracker.
766-
let _ = linked_chunk.updates_as_vector_diffs();
767-
768682
let output = linked_chunk.debug_string();
769683

770684
assert_eq!(output.len(), 2);
771-
assert_eq!(&output[0], "chunk #0: events[#0: $12345678, #1: $2]");
685+
assert_eq!(&output[0], "chunk #0: events[$12345678, $2]");
772686
assert_eq!(&output[1], "chunk #1: gap['raclette']");
773687
}
774688

0 commit comments

Comments
 (0)