Skip to content

Commit

Permalink
Add an end bound for invalidating the cache of drawn timeline items (#49
Browse files Browse the repository at this point in the history 
)

* This avoids unnecessarily clearing the entire cache of drawn item indices
  upon a `Set` operation (e.g., an in-place update to a timeline event item).
* Also adds a `clear_cache` boolean to wipe the whole cache, which is a
  frequent enough occurrence that it merits an dedicated optimization.
  • Loading branch information
@kevinaboos
kevinaboos authored Feb 21, 2024
1 parent 6b3a48d commit cafc78b
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 67 deletions.
58 changes: 21 additions & 37 deletions src/home/room_screen.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! A room screen is the UI page that displays a single Room's timeline of events/messages
//! along with a message input bar at the bottom.
use std::{collections::BTreeMap, ops::DerefMut, sync::{Arc, Mutex}};
use std::{collections::BTreeMap, ops::{DerefMut, Range}, sync::{Arc, Mutex}};

use imbl::Vector;
use makepad_widgets::*;
Expand Down Expand Up @@ -604,10 +604,13 @@ pub enum TimelineUpdate {
NewItems {
/// The entire list of timeline items (events) for a room.
items: Vector<Arc<TimelineItem>>,
/// The index of the first item in the `items` list that has changed.
/// Any items before this index are assumed to be unchanged and need not be redrawn,
/// while any items after this index are assumed to be changed and must be redrawn.
index_of_first_change: usize,
/// The range of indices in the `items` list that have been changed in this update
/// and thus must be removed from any caches of drawn items in the timeline.
/// Any items outside of this range are assumed to be unchanged and need not be redrawn.
changed_indices: Range<usize>,
/// Whether to clear the entire cache of drawn items in the timeline.
/// This supercedes `index_of_first_change` and is used when the entire timeline is being redrawn.
clear_cache: bool,
},
/// A notice that the start of the timeline has been reached, meaning that
/// there is no need to send further backwards pagination requests.
Expand Down Expand Up @@ -793,21 +796,28 @@ impl Widget for Timeline {
let mut done_loading = false;
while let Ok(update) = tl.update_receiver.try_recv() {
match update {
TimelineUpdate::NewItems { items, index_of_first_change } => {
TimelineUpdate::NewItems { items, changed_indices, clear_cache } => {
// Determine which item is currently visible the top of the screen
// so that we can jump back to that position instantly after applying this update.
if let Some(top_event_id) = tl.items.get(orig_first_id).map(|item| item.unique_id()) {
for (idx, item) in items.iter().enumerate() {
if item.unique_id() == top_event_id {
log!("Timeline::handle_event(): jumping view from top event index {orig_first_id} to index {idx}");
portal_list.set_first_id(idx);
if orig_first_id != idx {
log!("Timeline::handle_event(): jumping view from top event index {orig_first_id} to index {idx}");
portal_list.set_first_id(idx);
}
break;
}
}
}
tl.content_drawn_since_last_update.remove(index_of_first_change .. items.len());
tl.profile_drawn_since_last_update.remove(index_of_first_change .. items.len());
// log!("Timeline::handle_event(): index_of_first_change: {index_of_first_change}, items len: {}\ncontent drawn: {:#?}\nprofile drawn: {:#?}", items.len(), tl.content_drawn_since_last_update, tl.profile_drawn_since_last_update);
if clear_cache {
tl.content_drawn_since_last_update.clear();
tl.profile_drawn_since_last_update.clear();
} else {
tl.content_drawn_since_last_update.remove(changed_indices.clone());
tl.profile_drawn_since_last_update.remove(changed_indices.clone());
// log!("Timeline::handle_event(): changed_indices: {changed_indices:?}, items len: {}\ncontent drawn: {:#?}\nprofile drawn: {:#?}", items.len(), tl.content_drawn_since_last_update, tl.profile_drawn_since_last_update);
}
tl.items = items;
}
TimelineUpdate::TimelineStartReached => {
Expand Down Expand Up @@ -1015,32 +1025,6 @@ impl ItemDrawnStatus {
}
}

// TODO: return this `ItemDrawnStatus` from the populate_*_view functions and use it to determine
// if that item ID can be added to the `drawn_since_last_update` range set (only if both are true).
// For now, we should only add items that are fully drawn to the range set,
// as we don't want to accidentally miss redrawing updated items that were only partially drawn.
// In this way, we won't consider an item fully drawn until both its profile and content are fully drawn.
// ****
// Note: we'll also need to differentiate between:
// an avatar not existing at all (considered fully drawn)
// vs an avatar not being "ready" or not being fetched yet (considered not fully drawn)

// ****
// Also, we should split `drawn_since_last_update` into two separate `RangeSet`s:
// -- one for items whose CONTENT has been drawn fully, and
// -- one for items whose PROFILE has been drawn fully.
// This way, we can redraw the profile of an item without redrawing its content, and vice versa --> efficient!
// ****
// We should also use a range to specify `index_of_first_change` AND index of last change,
// such that we can support diff operations like set (editing/updating a single event).
// To do so, we'll have to send interim message updates to the UI thread rather than always sending the entire batch of diffs,
// but that's no problem because sending those updates is already very cheap.
// Plus, we already have plans to split up the batches across multiple update messages in the future,
// in order to support conveying more detailed info about which items were actually changed and at which indices
// (e.g., we'll eventually send one update per contiguous set of changed items, rather than one update per entire batch of items).




/// Creates, populates, and adds a Message liveview widget to the given `PortalList`
/// with the given `item_id`.
Expand Down
99 changes: 69 additions & 30 deletions src/sliding_sync.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use anyhow::{Result, bail};
use clap::Parser;
use eyeball_im::VectorDiff;
use futures_util::{StreamExt, pin_mut};
use imbl::Vector;
use makepad_widgets::{SignalToUI, error, log};
use matrix_sdk::{
Client,
Expand All @@ -20,13 +21,13 @@ use matrix_sdk::{
media::MediaRequest,
Room,
};
use matrix_sdk_ui::{timeline::{SlidingSyncRoomExt, PaginationOptions, BackPaginationStatus, TimelineItemContent, AnyOtherFullStateEventContent}, Timeline};
use matrix_sdk_ui::{timeline::{AnyOtherFullStateEventContent, BackPaginationStatus, PaginationOptions, SlidingSyncRoomExt, TimelineItem, TimelineItemContent}, Timeline};
use tokio::{
runtime::Handle,
sync::mpsc::{UnboundedSender, UnboundedReceiver}, task::JoinHandle,
};
use unicode_segmentation::UnicodeSegmentation;
use std::{cmp::min, collections::BTreeMap, sync::{Arc, Mutex, OnceLock}};
use std::{cmp::{max, min}, collections::BTreeMap, ops::Range, sync::{Arc, Mutex, OnceLock}};
use url::Url;

use crate::{home::{rooms_list::{self, RoomPreviewEntry, RoomsListUpdate, RoomPreviewAvatar, enqueue_rooms_list_update}, room_screen::TimelineUpdate}, media_cache::MediaCacheEntry, utils::MEDIA_THUMBNAIL_FORMAT};
Expand Down Expand Up @@ -703,84 +704,122 @@ async fn timeline_subscriber_handler(

sender.send(TimelineUpdate::NewItems {
items: timeline_items.clone(),
index_of_first_change: 0,
changed_indices: usize::MIN..usize::MAX,
clear_cache: true,
}).expect("Error: timeline update sender couldn't send update with initial items!");

let send_update = |timeline_items: Vector<Arc<TimelineItem>>, changed_indices: Range<usize>, clear_cache: bool, num_updates: usize| {
if num_updates > 0 {
// log!("timeline_subscriber: applied {num_updates} updates for room {room_id}. Clear cache? {clear_cache}. Changes: {changed_indices:?}.");
sender.send(TimelineUpdate::NewItems {
items: timeline_items,
changed_indices,
clear_cache,
}).expect("Error: timeline update sender couldn't send update with new items!");

// Send a Makepad-level signal to update this room's timeline UI view.
SignalToUI::set_ui_signal();
}
};

const LOG_DIFFS: bool = false;

while let Some(batch) = subscriber.next().await {
let num_updates = batch.len();
let mut num_updates = 0;
let mut index_of_first_change = usize::MAX;
let mut index_of_last_change = usize::MIN;
let mut clear_cache = false; // whether to clear the entire cache of items
for diff in batch {
match diff {
VectorDiff::Append { values } => {
let _values_len = values.len();
index_of_first_change = min(index_of_first_change, timeline_items.len());
if LOG_DIFFS { log!("timeline_subscriber: diff Append {}, index_of_first_change: {index_of_first_change}", values.len()); }
timeline_items.extend(values);
index_of_last_change = max(index_of_last_change, timeline_items.len());
if LOG_DIFFS { log!("timeline_subscriber: diff Append {_values_len}. Changes: {index_of_first_change}..{index_of_last_change}"); }
num_updates += 1;
}
VectorDiff::Clear => {
if LOG_DIFFS { log!("timeline_subscriber: diff Clear"); }
index_of_first_change = 0;
clear_cache = true;
timeline_items.clear();
num_updates += 1;
}
VectorDiff::PushFront { value } => {
if LOG_DIFFS { log!("timeline_subscriber: diff PushFront"); }
index_of_first_change = 0;
clear_cache = true;
timeline_items.push_front(value);
num_updates += 1;
}
VectorDiff::PushBack { value } => {
if LOG_DIFFS { log!("timeline_subscriber: diff PushBack"); }
index_of_first_change = min(index_of_first_change, timeline_items.len());
timeline_items.push_back(value);
index_of_last_change = max(index_of_last_change, timeline_items.len());
if LOG_DIFFS { log!("timeline_subscriber: diff PushBack. Changes: {index_of_first_change}..{index_of_last_change}"); }
num_updates += 1;
}
VectorDiff::PopFront => {
if LOG_DIFFS { log!("timeline_subscriber: diff PopFront"); }
index_of_first_change = 0;
clear_cache = true;
timeline_items.pop_front();
num_updates += 1;
}
VectorDiff::PopBack => {
if LOG_DIFFS { log!("timeline_subscriber: diff PopBack"); }
timeline_items.pop_back();
index_of_first_change = min(index_of_first_change, timeline_items.len().saturating_sub(1));
index_of_first_change = min(index_of_first_change, timeline_items.len());
index_of_last_change = usize::MAX;
if LOG_DIFFS { log!("timeline_subscriber: diff PopBack. Changes: {index_of_first_change}..{index_of_last_change}"); }
num_updates += 1;
}
VectorDiff::Insert { index, value } => {
if LOG_DIFFS { log!("timeline_subscriber: diff Insert at {index}"); }
index_of_first_change = min(index_of_first_change, index);
if index == 0 {
clear_cache = true;
} else {
index_of_first_change = min(index_of_first_change, index);
index_of_last_change = usize::MAX;
}
timeline_items.insert(index, value);
if LOG_DIFFS { log!("timeline_subscriber: diff Insert at {index}. Changes: {index_of_first_change}..{index_of_last_change}"); }
num_updates += 1;
}
VectorDiff::Set { index, value } => {
if LOG_DIFFS { log!("timeline_subscriber: diff Set at {index}"); }
index_of_first_change = min(index_of_first_change, index);
index_of_last_change = max(index_of_last_change, index.saturating_add(1));
timeline_items.set(index, value);
if LOG_DIFFS { log!("timeline_subscriber: diff Set at {index}. Changes: {index_of_first_change}..{index_of_last_change}"); }
num_updates += 1;
}
VectorDiff::Remove { index } => {
if LOG_DIFFS { log!("timeline_subscriber: diff Remove at {index}"); }
index_of_first_change = min(index_of_first_change, index.saturating_sub(1));
if index == 0 {
clear_cache = true;
} else {
index_of_first_change = min(index_of_first_change, index.saturating_sub(1));
index_of_last_change = usize::MAX;
}
timeline_items.remove(index);
if LOG_DIFFS { log!("timeline_subscriber: diff Remove at {index}. Changes: {index_of_first_change}..{index_of_last_change}"); }
num_updates += 1;
}
VectorDiff::Truncate { length } => {
if LOG_DIFFS { log!("timeline_subscriber: diff Truncate to length {length}"); }
index_of_first_change = min(index_of_first_change, length.saturating_sub(1));
if length == 0 {
clear_cache = true;
} else {
index_of_first_change = min(index_of_first_change, length.saturating_sub(1));
index_of_last_change = usize::MAX;
}
timeline_items.truncate(length);
if LOG_DIFFS { log!("timeline_subscriber: diff Truncate to length {length}. Changes: {index_of_first_change}..{index_of_last_change}"); }
num_updates += 1;
}
VectorDiff::Reset { values } => {
if LOG_DIFFS { log!("timeline_subscriber: diff Reset, new length {}", values.len()); }
index_of_first_change = 0; // we must assume that all items have changed.
clear_cache = true; // we must assume all items have changed.
timeline_items = values;
num_updates += 1;
}
}
}
if num_updates > 0 {
log!("timeline_subscriber: applied {num_updates} updates for room {room_id}; first change at index {index_of_first_change}.");

sender.send(TimelineUpdate::NewItems {
items: timeline_items.clone(),
index_of_first_change,
}).expect("Error: timeline update sender couldn't send update with new items!");

// Send a Makepad-level signal to update this room's timeline UI view.
SignalToUI::set_ui_signal();
}
send_update(timeline_items.clone(), index_of_first_change..index_of_last_change, clear_cache, num_updates);
}

error!("Error: unexpectedly ended timeline subscriber for room {room_id}.");
Expand Down

0 comments on commit cafc78b

Please sign in to comment.