Browse Source

timeline: Attempt to minimize the diff batches from the SDK

A recent change in the SDK made them less optimized than before,
it creates jumps in the room history when sending a message.

We try to work around that by optimizing the batch ourselves.
pipelines/786320
Kévin Commaille 1 year ago
parent
commit
2d0552b74d
No known key found for this signature in database
GPG Key ID: C971D9DBC9D678D
  1. 7
      Cargo.lock
  2. 1
      Cargo.toml
  3. 2
      src/prelude.rs
  4. 29
      src/session/model/room/event/mod.rs
  5. 239
      src/session/model/room/timeline/mod.rs
  6. 60
      src/session/model/room/timeline/timeline_item.rs
  7. 34
      src/session/model/room/timeline/virtual_item.rs

7
Cargo.lock generated

@ -1026,6 +1026,12 @@ dependencies = [
"syn",
]
[[package]]
name = "diff"
version = "0.1.13"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "56254986775e3233ffa9c4d7d3faaf6d36a2c09d30b20687e9f88bc8bafc16c8"
[[package]]
name = "digest"
version = "0.10.7"
@ -1349,6 +1355,7 @@ name = "fractal"
version = "10.0.0-beta"
dependencies = [
"ashpd",
"diff",
"djb_hash",
"futures-channel",
"futures-util",

1
Cargo.toml

@ -21,6 +21,7 @@ codegen-units = 16
# Please keep dependencies sorted.
[dependencies]
diff = "0.1"
djb_hash = "0.1"
futures-channel = "0.3"
futures-util = "0.3"

2
src/prelude.rs

@ -4,7 +4,7 @@ pub(crate) use crate::{
ToastableDialogImpl,
},
contrib::CameraExt,
session::model::UserExt,
session::model::{TimelineItemExt, UserExt},
session_list::SessionInfoExt,
user_facing_error::UserFacingError,
utils::{

29
src/session/model/room/event/mod.rs

@ -182,10 +182,6 @@ mod imp {
}
impl TimelineItemImpl for Event {
fn id(&self) -> String {
format!("Event::{:?}", self.identifier())
}
fn can_hide_header(&self) -> bool {
self.item().content().can_show_header()
}
@ -509,9 +505,10 @@ glib::wrapper! {
impl Event {
/// Create a new `Event` in the given room with the given SDK timeline item.
pub fn new(item: EventTimelineItem, room: &Room) -> Self {
pub fn new(item: EventTimelineItem, room: &Room, timeline_id: &str) -> Self {
let obj = glib::Object::builder::<Self>()
.property("room", room)
.property("timeline-id", timeline_id)
.build();
obj.imp().set_item(item);
@ -522,26 +519,14 @@ impl Event {
/// Try to update this event with the given SDK timeline item.
///
/// Returns `true` if the update succeeded.
pub(crate) fn try_update_with(&self, item: &EventTimelineItem) -> bool {
let imp = self.imp();
pub(crate) fn try_update_with(&self, item: &EventTimelineItem, timeline_id: &str) -> bool {
let is_same_item = self.timeline_id() == timeline_id;
match &self.identifier() {
TimelineEventItemId::TransactionId(txn_id)
if item.transaction_id().is_some_and(|id| id == txn_id) =>
{
imp.set_item(item.clone());
return true;
}
TimelineEventItemId::EventId(event_id)
if item.event_id().is_some_and(|id| id == event_id) =>
{
imp.set_item(item.clone());
return true;
}
_ => {}
if is_same_item {
self.imp().set_item(item.clone());
}
false
is_same_item
}
/// The underlying SDK timeline item.

239
src/session/model/room/timeline/mod.rs

@ -1,7 +1,11 @@
mod timeline_item;
mod virtual_item;
use std::{collections::HashMap, ops::ControlFlow, sync::Arc};
use std::{
collections::{HashMap, VecDeque},
ops::ControlFlow,
sync::Arc,
};
use futures_util::StreamExt;
use gtk::{
@ -11,7 +15,7 @@ use gtk::{
subclass::prelude::*,
};
use matrix_sdk_ui::{
eyeball_im::VectorDiff,
eyeball_im::{Vector, VectorDiff},
timeline::{
default_event_filter, RoomExt, Timeline as SdkTimeline, TimelineEventItemId,
TimelineItem as SdkTimelineItem,
@ -28,7 +32,7 @@ use tokio::task::AbortHandle;
use tracing::error;
pub(crate) use self::{
timeline_item::{TimelineItem, TimelineItemImpl},
timeline_item::{TimelineItem, TimelineItemExt, TimelineItemImpl},
virtual_item::{VirtualItem, VirtualItemKind},
};
use super::{Event, Room};
@ -208,16 +212,16 @@ mod imp {
let matrix_timeline = Arc::new(matrix_timeline);
self.matrix_timeline
.set(matrix_timeline.clone())
.expect("matrix timeline was uninitialized");
.expect("matrix timeline is uninitialized");
let (values, timeline_stream) = matrix_timeline.subscribe().await;
let (values, timeline_stream) = matrix_timeline.subscribe_batched().await;
if !values.is_empty() {
self.update(VectorDiff::Append { values });
self.update_with_single_diff(VectorDiff::Append { values }, &room);
}
let obj_weak = glib::SendWeakRef::from(self.obj().downgrade());
let fut = timeline_stream.for_each(move |diff| {
let fut = timeline_stream.for_each(move |diff_list| {
let obj_weak = obj_weak.clone();
let room_id = room_id.clone();
async move {
@ -225,7 +229,7 @@ mod imp {
ctx.spawn(async move {
spawn!(async move {
if let Some(obj) = obj_weak.upgrade() {
obj.imp().update(diff);
obj.imp().update_with_diff_list(diff_list);
} else {
error!(
"Could not send timeline diff for room {room_id}: \
@ -238,7 +242,9 @@ mod imp {
});
let diff_handle = spawn_tokio!(fut);
self.diff_handle.set(diff_handle.abort_handle()).unwrap();
self.diff_handle
.set(diff_handle.abort_handle())
.expect("handle is uninitialized");
self.watch_read_receipts().await;
self.set_state(TimelineState::Ready);
@ -266,15 +272,148 @@ mod imp {
self.obj().notify_has_room_create();
}
/// Update this `Timeline` with the given diff.
#[allow(clippy::too_many_lines)]
fn update(&self, diff: VectorDiff<Arc<SdkTimelineItem>>) {
/// Update this timeline with the given diff list.
fn update_with_diff_list(&self, diff_list: Vec<VectorDiff<Arc<SdkTimelineItem>>>) {
let Some(room) = self.room.upgrade() else {
return;
};
let sdk_items = &self.sdk_items;
let was_empty = self.is_empty();
let diff_list = self.minimize_diff_list(diff_list);
for diff in diff_list {
self.update_with_single_diff(diff, &room);
}
let obj = self.obj();
if self.is_empty() != was_empty {
obj.notify_is_empty();
}
obj.emit_read_change_trigger();
}
/// Attempt to minimize the given list of diffs.
///
/// This is necessary because the SDK diffs are not always optimized,
/// e.g. an item is removed then re-added, which creates jumps in the
/// room history.
fn minimize_diff_list(
&self,
diff_list: Vec<VectorDiff<Arc<SdkTimelineItem>>>,
) -> Vec<VectorDiff<Arc<SdkTimelineItem>>> {
let mut old_items = Vec::with_capacity(self.sdk_items.n_items() as usize);
// Construct the current state.
for item in self.sdk_items.iter::<TimelineItem>() {
let Ok(item) = item else {
// The list changed, return early.
return diff_list;
};
old_items.push(item.timeline_id());
}
// Get the new state by applying the diffs.
let mut value_map = HashMap::with_capacity(diff_list.len());
let mut new_items = old_items.iter().cloned().collect::<VecDeque<_>>();
for diff in &diff_list {
match diff {
VectorDiff::Append { values } => {
new_items.reserve(values.len());
for value in values {
let timeline_id = value.unique_id().0.clone();
value_map.insert(timeline_id.clone(), value.clone());
new_items.push_back(timeline_id);
}
}
VectorDiff::PushFront { value } => {
let timeline_id = value.unique_id().0.clone();
value_map.insert(timeline_id.clone(), value.clone());
new_items.push_front(timeline_id);
}
VectorDiff::PushBack { value } => {
let timeline_id = value.unique_id().0.clone();
value_map.insert(timeline_id.clone(), value.clone());
new_items.push_back(timeline_id);
}
VectorDiff::PopFront => {
new_items.pop_front();
}
VectorDiff::PopBack => {
new_items.pop_back();
}
VectorDiff::Insert { index, value } => {
let timeline_id = value.unique_id().0.clone();
value_map.insert(timeline_id.clone(), value.clone());
new_items.insert(*index, timeline_id);
}
VectorDiff::Set { index, value } => {
let timeline_id = value.unique_id().0.clone();
value_map.insert(timeline_id.clone(), value.clone());
let item = new_items.get_mut(*index).expect("item exists");
*item = timeline_id;
}
VectorDiff::Remove { index } => {
new_items.remove(*index);
}
VectorDiff::Clear | VectorDiff::Truncate { .. } | VectorDiff::Reset { .. } => {
// Minimizing this will probably make a worse diff, so we return early.
return diff_list;
}
}
}
let new_items = Vec::from(new_items);
let mut min_diff_list = Vec::with_capacity(diff_list.len());
let mut index = 0;
let mut n_items = old_items.len();
for result in diff::slice(&old_items, &new_items) {
match result {
diff::Result::Left(_) => {
min_diff_list.push(VectorDiff::Remove { index });
n_items -= 1;
}
diff::Result::Both(timeline_id, _) => {
if let Some(value) = value_map.get(timeline_id).cloned() {
min_diff_list.push(VectorDiff::Set { index, value });
}
index += 1;
}
diff::Result::Right(timeline_id) => {
let value = value_map
.get(timeline_id)
.expect("value exists for added item")
.clone();
if index == n_items {
if let Some(VectorDiff::Append { values }) = min_diff_list.last_mut() {
values.push_back(value);
} else {
min_diff_list.push(VectorDiff::Append {
values: Vector::from([value]),
});
}
} else {
min_diff_list.push(VectorDiff::Insert { index, value });
}
index += 1;
n_items += 1;
}
}
}
min_diff_list
}
/// Update this timeline with the given diff.
#[allow(clippy::too_many_lines)]
fn update_with_single_diff(&self, diff: VectorDiff<Arc<SdkTimelineItem>>, room: &Room) {
match diff {
VectorDiff::Append { values } => {
let new_list = values
@ -282,10 +421,10 @@ mod imp {
.map(|item| self.create_item(&item))
.collect::<Vec<_>>();
let pos = sdk_items.n_items();
let pos = self.sdk_items.n_items();
let added = new_list.len() as u32;
sdk_items.extend_from_slice(&new_list);
self.sdk_items.extend_from_slice(&new_list);
self.update_items_headers(pos, added.max(1));
// Try to update the latest unread message.
@ -294,14 +433,14 @@ mod imp {
);
}
VectorDiff::Clear => {
sdk_items.remove_all();
self.sdk_items.remove_all();
self.event_map.borrow_mut().clear();
self.set_has_room_create(false);
}
VectorDiff::PushFront { value } => {
let item = self.create_item(&value);
sdk_items.insert(0, &item);
self.sdk_items.insert(0, &item);
self.update_items_headers(0, 1);
// Try to update the latest unread message.
@ -311,8 +450,8 @@ mod imp {
}
VectorDiff::PushBack { value } => {
let item = self.create_item(&value);
let pos = sdk_items.n_items();
sdk_items.append(&item);
let pos = self.sdk_items.n_items();
self.sdk_items.append(&item);
self.update_items_headers(pos, 1);
// Try to update the latest unread message.
@ -321,24 +460,32 @@ mod imp {
}
}
VectorDiff::PopFront => {
let item = sdk_items.item(0).and_downcast().unwrap();
let item = self
.sdk_items
.item(0)
.and_downcast()
.expect("all items are TimelineItems");
self.remove_item(&item);
sdk_items.remove(0);
self.sdk_items.remove(0);
self.update_items_headers(0, 1);
}
VectorDiff::PopBack => {
let pos = sdk_items.n_items() - 1;
let item = sdk_items.item(pos).and_downcast().unwrap();
let pos = self.sdk_items.n_items() - 1;
let item = self
.sdk_items
.item(pos)
.and_downcast()
.expect("all items are TimelineItems");
self.remove_item(&item);
sdk_items.remove(pos);
self.sdk_items.remove(pos);
}
VectorDiff::Insert { index, value } => {
let pos = index as u32;
let item = self.create_item(&value);
sdk_items.insert(pos, &item);
self.sdk_items.insert(pos, &item);
self.update_items_headers(pos, 1);
// Try to update the latest unread message.
@ -348,7 +495,11 @@ mod imp {
}
VectorDiff::Set { index, value } => {
let pos = index as u32;
let prev_item = sdk_items.item(pos).and_downcast::<TimelineItem>().unwrap();
let prev_item = self
.sdk_items
.item(pos)
.and_downcast::<TimelineItem>()
.expect("all items are TimelineItems");
let item = if prev_item.try_update_with(&value) {
if let Some(event) = prev_item.downcast_ref::<Event>() {
@ -364,7 +515,7 @@ mod imp {
self.remove_item(&prev_item);
let item = self.create_item(&value);
sdk_items.splice(pos, 1, &[item.clone()]);
self.sdk_items.splice(pos, 1, &[item.clone()]);
item
};
@ -379,22 +530,31 @@ mod imp {
}
VectorDiff::Remove { index } => {
let pos = index as u32;
let item = sdk_items.item(pos).and_downcast().unwrap();
let item = self
.sdk_items
.item(pos)
.and_downcast()
.expect("all items are TimelineItems");
self.remove_item(&item);
sdk_items.remove(pos);
self.sdk_items.remove(pos);
self.update_items_headers(pos, 1);
}
VectorDiff::Truncate { length } => {
let new_len = length as u32;
let old_len = sdk_items.n_items();
let old_len = self.sdk_items.n_items();
for pos in new_len..old_len {
let item = sdk_items.item(pos).and_downcast().unwrap();
let item = self
.sdk_items
.item(pos)
.and_downcast()
.expect("all items are TimelineItems");
self.remove_item(&item);
}
sdk_items.splice(new_len, old_len - new_len, &[] as &[glib::Object]);
self.sdk_items
.splice(new_len, old_len - new_len, &[] as &[glib::Object]);
}
VectorDiff::Reset { values } => {
// Reset the state.
@ -406,10 +566,10 @@ mod imp {
.map(|item| self.create_item(&item))
.collect::<Vec<_>>();
let removed = sdk_items.n_items();
let removed = self.sdk_items.n_items();
let added = new_list.len() as u32;
sdk_items.splice(0, removed, &new_list);
self.sdk_items.splice(0, removed, &new_list);
self.update_items_headers(0, added.max(1));
// Try to update the latest unread message.
@ -418,13 +578,6 @@ mod imp {
);
}
}
let obj = self.obj();
if self.is_empty() != was_empty {
obj.notify_is_empty();
}
obj.emit_read_change_trigger();
}
/// Update `nb` items' headers starting at `pos`.
@ -625,7 +778,7 @@ mod imp {
let handle = spawn_tokio!(fut);
self.read_receipts_changed_handle
.set(handle.abort_handle())
.unwrap();
.expect("handle is uninitialized");
}
}
}
@ -738,7 +891,7 @@ impl Timeline {
.await
})
.await
.unwrap();
.expect("task was not aborted");
let sdk_items = &self.imp().sdk_items;
let count = sdk_items.n_items();

60
src/session/model/room/timeline/timeline_item.rs

@ -6,14 +6,16 @@ use super::VirtualItem;
use crate::session::model::{Event, Room};
mod imp {
use std::{cell::Cell, marker::PhantomData};
use std::{
cell::{Cell, RefCell},
marker::PhantomData,
};
use super::*;
#[repr(C)]
pub struct TimelineItemClass {
parent_class: glib::object::ObjectClass,
pub(super) id: fn(&super::TimelineItem) -> String,
pub(super) selectable: fn(&super::TimelineItem) -> bool,
pub(super) can_hide_header: fn(&super::TimelineItem) -> bool,
pub(super) event_sender_id: fn(&super::TimelineItem) -> Option<OwnedUserId>,
@ -23,11 +25,6 @@ mod imp {
type Type = TimelineItem;
}
pub(super) fn timeline_item_id(this: &super::TimelineItem) -> String {
let klass = this.class();
(klass.as_ref().id)(this)
}
pub(super) fn timeline_item_selectable(this: &super::TimelineItem) -> bool {
let klass = this.class();
(klass.as_ref().selectable)(this)
@ -46,11 +43,9 @@ mod imp {
#[derive(Debug, Default, glib::Properties)]
#[properties(wrapper_type = super::TimelineItem)]
pub struct TimelineItem {
/// A unique ID for this `TimelineItem`.
///
/// For debugging purposes.
#[property(get = Self::id)]
id: PhantomData<String>,
/// A unique ID for this `TimelineItem` in the local timeline.
#[property(get, construct_only)]
timeline_id: RefCell<String>,
/// Whether this `TimelineItem` is selectable.
///
/// Defaults to `false`.
@ -85,13 +80,6 @@ mod imp {
impl ObjectImpl for TimelineItem {}
impl TimelineItem {
/// A unique ID for this `TimelineItem`.
///
/// For debugging purposes.
fn id(&self) -> String {
imp::timeline_item_id(&self.obj())
}
/// Whether this `TimelineItem` is selectable.
///
/// Defaults to `false`.
@ -135,9 +123,11 @@ impl TimelineItem {
///
/// Constructs the proper child type.
pub fn new(item: &SdkTimelineItem, room: &Room) -> Self {
let timeline_id = &item.unique_id().0;
match item.kind() {
TimelineItemKind::Event(event) => Event::new(event.clone(), room).upcast(),
TimelineItemKind::Virtual(item) => VirtualItem::new(item).upcast(),
TimelineItemKind::Event(event) => Event::new(event.clone(), room, timeline_id).upcast(),
TimelineItemKind::Virtual(item) => VirtualItem::with_item(item, timeline_id).upcast(),
}
}
@ -145,14 +135,16 @@ impl TimelineItem {
///
/// Returns `true` if the update succeeded.
pub(crate) fn try_update_with(&self, item: &SdkTimelineItem) -> bool {
let timeline_id = &item.unique_id().0;
match item.kind() {
TimelineItemKind::Event(new_event) => {
if let Some(event) = self.downcast_ref::<Event>() {
return event.try_update_with(new_event);
return event.try_update_with(new_event, timeline_id);
}
}
TimelineItemKind::Virtual(_item) => {
// Always invalidate. It shouldn't happen often and updating
// Always invalidate. It should not happen often and updating
// those should be unexpensive.
}
}
@ -168,10 +160,8 @@ impl TimelineItem {
/// of `TimelineItemImpl`.
#[allow(dead_code)]
pub(crate) trait TimelineItemExt: 'static {
/// A unique ID for this `TimelineItem`.
///
/// For debugging purposes.
fn id(&self) -> String;
/// A unique ID for this `TimelineItem` in the local timeline.
fn timeline_id(&self) -> String;
/// Whether this `TimelineItem` is selectable.
///
@ -198,8 +188,8 @@ pub(crate) trait TimelineItemExt: 'static {
}
impl<O: IsA<TimelineItem>> TimelineItemExt for O {
fn id(&self) -> String {
self.upcast_ref().id()
fn timeline_id(&self) -> String {
self.upcast_ref().timeline_id()
}
fn selectable(&self) -> bool {
@ -229,8 +219,6 @@ impl<O: IsA<TimelineItem>> TimelineItemExt for O {
/// Overriding a method from this Trait overrides also its behavior in
/// `TimelineItemExt`.
pub(crate) trait TimelineItemImpl: ObjectImpl {
fn id(&self) -> String;
fn selectable(&self) -> bool {
false
}
@ -255,7 +243,6 @@ where
let klass = class.as_mut();
klass.id = id_trampoline::<T>;
klass.selectable = selectable_trampoline::<T>;
klass.can_hide_header = can_hide_header_trampoline::<T>;
klass.event_sender_id = event_sender_id_trampoline::<T>;
@ -263,15 +250,6 @@ where
}
// Virtual method implementation trampolines.
fn id_trampoline<T>(this: &TimelineItem) -> String
where
T: ObjectSubclass + TimelineItemImpl,
T::Type: IsA<TimelineItem>,
{
let this = this.downcast_ref::<T::Type>().unwrap();
this.imp().id()
}
fn selectable_trampoline<T>(this: &TimelineItem) -> bool
where
T: ObjectSubclass + TimelineItemImpl,

34
src/session/model/room/timeline/virtual_item.rs

@ -67,19 +67,7 @@ mod imp {
#[glib::derived_properties]
impl ObjectImpl for VirtualItem {}
impl TimelineItemImpl for VirtualItem {
fn id(&self) -> String {
match &**self.kind.borrow() {
VirtualItemKind::Spinner => "VirtualItem::Spinner".to_owned(),
VirtualItemKind::Typing => "VirtualItem::Typing".to_owned(),
VirtualItemKind::TimelineStart => "VirtualItem::TimelineStart".to_owned(),
VirtualItemKind::DayDivider(date) => {
format!("VirtualItem::DayDivider({})", date.format("%F").unwrap())
}
VirtualItemKind::NewMessages => "VirtualItem::NewMessages".to_owned(),
}
}
}
impl TimelineItemImpl for VirtualItem {}
}
glib::wrapper! {
@ -91,10 +79,12 @@ glib::wrapper! {
impl VirtualItem {
/// Create a new `VirtualItem` from a virtual timeline item.
pub fn new(item: &VirtualTimelineItem) -> Self {
pub fn with_item(item: &VirtualTimelineItem, timeline_id: &str) -> Self {
match item {
VirtualTimelineItem::DateDivider(ts) => Self::day_divider_with_timestamp(*ts),
VirtualTimelineItem::ReadMarker => Self::new_messages(),
VirtualTimelineItem::DateDivider(ts) => {
Self::day_divider_with_timestamp(*ts, timeline_id)
}
VirtualTimelineItem::ReadMarker => Self::new_messages(timeline_id),
}
}
@ -102,6 +92,7 @@ impl VirtualItem {
pub(crate) fn spinner() -> Self {
glib::Object::builder()
.property("kind", VirtualItemKind::Spinner.boxed())
.property("timeline-id", "VirtualItemKind::Spinner")
.build()
}
@ -109,6 +100,7 @@ impl VirtualItem {
pub(crate) fn typing() -> Self {
glib::Object::builder()
.property("kind", VirtualItemKind::Typing.boxed())
.property("timeline-id", "VirtualItemKind::Typing")
.build()
}
@ -116,13 +108,15 @@ impl VirtualItem {
pub(crate) fn timeline_start() -> Self {
glib::Object::builder()
.property("kind", VirtualItemKind::TimelineStart.boxed())
.property("timeline-id", "VirtualItemKind::TimelineStart")
.build()
}
/// Create a new messages virtual item.
pub(crate) fn new_messages() -> Self {
fn new_messages(timeline_id: &str) -> Self {
glib::Object::builder()
.property("kind", VirtualItemKind::NewMessages.boxed())
.property("timeline-id", timeline_id)
.build()
}
@ -133,13 +127,17 @@ impl VirtualItem {
/// current local time.
///
/// Panics if an error occurred when accessing the current local time.
pub(crate) fn day_divider_with_timestamp(timestamp: MilliSecondsSinceUnixEpoch) -> Self {
fn day_divider_with_timestamp(
timestamp: MilliSecondsSinceUnixEpoch,
timeline_id: &str,
) -> Self {
let date = glib::DateTime::from_unix_utc(timestamp.as_secs().into())
.or_else(|_| glib::DateTime::now_utc())
.expect("We should be able to get the current time");
glib::Object::builder()
.property("kind", VirtualItemKind::DayDivider(date).boxed())
.property("timeline-id", timeline_id)
.build()
}
}

Loading…
Cancel
Save