Skip to content

Commit d9880bd

Browse files
jmartinespjmartinesp
committed
fixup! contains several fixes for review comments, should be split into individual commits later
1 parent 24dc00f commit d9880bd

File tree

6 files changed

+173
-141
lines changed

6 files changed

+173
-141
lines changed

‎benchmarks/benches/room_bench.rs

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use std::time::Duration;
1+
use std::{sync::Arc, time::Duration};
22

33
use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Throughput};
44
use matrix_sdk::{
@@ -185,10 +185,17 @@ pub fn load_pinned_events_benchmark(c: &mut Criterion) {
185185
group.throughput(Throughput::Elements(count as u64));
186186
group.sample_size(10);
187187

188-
group.bench_function(BenchmarkId::new("load_pinned_events", name), |b| {
189-
b.to_async(&runtime).iter(|| async {
188+
let client = Arc::new(client);
189+
190+
{
191+
let client = client.clone();
192+
runtime.spawn_blocking(move || {
190193
client.event_cache().subscribe().unwrap();
194+
});
195+
}
191196

197+
group.bench_function(BenchmarkId::new("load_pinned_events", name), |b| {
198+
b.to_async(&runtime).iter(|| async {
192199
assert!(!room.pinned_event_ids().is_empty());
193200
assert_eq!(room.pinned_event_ids().len(), PINNED_EVENTS_COUNT);
194201

@@ -210,7 +217,7 @@ pub fn load_pinned_events_benchmark(c: &mut Criterion) {
210217
{
211218
let _guard = runtime.enter();
212219
runtime.block_on(server.reset());
213-
drop(client);
220+
// drop(client);
214221
drop(server);
215222
}
216223

@@ -249,6 +256,7 @@ fn criterion() -> Criterion {
249256
criterion_group! {
250257
name = room;
251258
config = criterion();
252-
targets = receive_all_members_benchmark, load_pinned_events_benchmark,
259+
targets = load_pinned_events_benchmark,
260+
// targets = receive_all_members_benchmark, load_pinned_events_benchmark,
253261
}
254262
criterion_main!(room);

‎crates/matrix-sdk-ui/src/timeline/pinned_events_loader.rs

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use std::{fmt::Formatter, sync::Arc};
1616

1717
use futures_util::future::join_all;
1818
use matrix_sdk::{
19-
config::RequestConfig, event_cache::paginator::PaginatorError, Room, SendOutsideWasm,
19+
config::RequestConfig, event_cache::paginator::PaginatorError, Error, Room, SendOutsideWasm,
2020
SyncOutsideWasm,
2121
};
2222
use matrix_sdk_base::deserialized_responses::SyncTimelineEvent;
@@ -25,7 +25,9 @@ use thiserror::Error;
2525
use tokio::sync::Semaphore;
2626
use tracing::{debug, warn};
2727

28-
use crate::timeline::event_handler::TimelineEventKind;
28+
use crate::timeline::{
29+
event_handler::TimelineEventKind, pinned_events_loader::PinnedEventsLoaderError::EventNotFound,
30+
};
2931

3032
const MAX_CONCURRENT_REQUESTS: usize = 10;
3133

@@ -82,7 +84,11 @@ impl PinnedEventsLoader {
8284
return None;
8385
}
8486
match provider.load_event_with_relations(&event_id, request_config).await {
85-
Ok(event) => Some(event),
87+
Ok((event, related_events)) => {
88+
let mut events = vec![event];
89+
events.extend(related_events);
90+
Some(events)
91+
}
8692
Err(err) => {
8793
warn!("error when loading pinned event: {err}");
8894
None
@@ -114,12 +120,12 @@ impl PinnedEventsLoader {
114120
#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)]
115121
pub trait PinnedEventsRoom: SendOutsideWasm + SyncOutsideWasm {
116122
/// Load a single room event from either the cache or network and any events
117-
/// related to it, if cached.
123+
/// related to it, if they are cached.
118124
async fn load_event_with_relations(
119125
&self,
120126
event_id: &EventId,
121127
request_config: Option<RequestConfig>,
122-
) -> Result<Vec<SyncTimelineEvent>, PaginatorError>;
128+
) -> Result<(SyncTimelineEvent, Vec<SyncTimelineEvent>), PaginatorError>;
123129

124130
/// Get the pinned event ids for a room.
125131
fn pinned_event_ids(&self) -> Vec<OwnedEventId>;
@@ -154,19 +160,24 @@ impl PinnedEventsRoom for Room {
154160
&self,
155161
event_id: &EventId,
156162
request_config: Option<RequestConfig>,
157-
) -> Result<Vec<SyncTimelineEvent>, PaginatorError> {
163+
) -> Result<(SyncTimelineEvent, Vec<SyncTimelineEvent>), PaginatorError> {
158164
if let Ok((cache, _handles)) = self.event_cache().await {
159-
let cached_event_with_relations = cache.event_with_relations(event_id).await;
160-
if !cached_event_with_relations.is_empty() {
165+
if let Some(ret) = cache.event_with_relations(event_id).await {
161166
debug!("Loaded pinned event {event_id} and related events from cache");
162-
return Ok(cached_event_with_relations);
167+
return Ok(ret);
163168
}
164169
}
165170

166171
debug!("Loading pinned event {event_id} from HS");
167172
self.event_with_context(event_id, true, uint!(0), request_config)
168173
.await
169-
.map(|e| if let Some(ev) = e.event { vec![ev.into()] } else { Vec::new() })
174+
.and_then(|e| {
175+
if let Some(ev) = e.event {
176+
Ok((ev.into(), Vec::new()))
177+
} else {
178+
Err(Error::UnknownError(Box::new(EventNotFound(event_id.to_owned()))))
179+
}
180+
})
170181
.map_err(|err| PaginatorError::SdkError(Box::new(err)))
171182
}
172183

‎crates/matrix-sdk-ui/src/timeline/tests/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,7 @@ impl PinnedEventsRoom for TestRoomDataProvider {
326326
&self,
327327
_event_id: &EventId,
328328
_request_config: Option<RequestConfig>,
329-
) -> Result<Vec<SyncTimelineEvent>, PaginatorError> {
329+
) -> Result<(SyncTimelineEvent, Vec<SyncTimelineEvent>), PaginatorError> {
330330
unimplemented!();
331331
}
332332

‎crates/matrix-sdk-ui/tests/integration/timeline/pinned_event.rs

Lines changed: 16 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -367,10 +367,9 @@ async fn test_edited_events_are_reflected_in_sync() {
367367
test_helper.server.reset().await;
368368

369369
// Load new pinned event contents from sync, where $2 is and edit on $1
370-
let _ = Box::pin(
371-
test_helper.setup_sync_response(vec![(edit("$2", "$1", "edited message!"), true)], None),
372-
)
373-
.await;
370+
let _ = test_helper
371+
.setup_sync_response(vec![(edit("$2", "$1", "edited message!"), true)], None)
372+
.await;
374373

375374
// The list is reloaded, so it's reset
376375
assert_matches!(timeline_stream.next().await.unwrap(), VectorDiff::Clear);
@@ -434,7 +433,7 @@ async fn test_redacted_events_are_reflected_in_sync() {
434433
test_helper.server.reset().await;
435434

436435
// Load new pinned event contents from sync, where $1 is now redacted
437-
let _ = Box::pin(test_helper.setup_sync_response(vec![(redact("$2", "$1"), true)], None)).await;
436+
let _ = test_helper.setup_sync_response(vec![(redact("$2", "$1"), true)], None).await;
438437

439438
// The list is reloaded, so it's reset
440439
assert_matches!(timeline_stream.next().await.unwrap(), VectorDiff::Clear);
@@ -494,10 +493,9 @@ async fn test_edited_events_survive_pinned_event_ids_change() {
494493

495494
// Load new pinned event contents from sync, $2 was pinned but wasn't available
496495
// before
497-
let _ = Box::pin(
498-
test_helper.setup_sync_response(vec![(edit("$2", "$1", "edited message!"), true)], None),
499-
)
500-
.await;
496+
let _ = test_helper
497+
.setup_sync_response(vec![(edit("$2", "$1", "edited message!"), true)], None)
498+
.await;
501499
test_helper.server.reset().await;
502500

503501
// The list is reloaded, so it's reset
@@ -523,11 +521,9 @@ async fn test_edited_events_survive_pinned_event_ids_change() {
523521
assert_pending!(timeline_stream);
524522

525523
// Load new pinned event contents from sync: $3
526-
let _ = Box::pin(
527-
test_helper
528-
.setup_sync_response(vec![(msg("$3", "new message"), true)], Some(vec!["$1", "$3"])),
529-
)
530-
.await;
524+
let _ = test_helper
525+
.setup_sync_response(vec![(msg("$3", "new message"), true)], Some(vec!["$1", "$3"]))
526+
.await;
531527
test_helper.server.reset().await;
532528

533529
// New item gets added
@@ -604,18 +600,14 @@ impl TestHelper {
604600
) -> Result<SyncResponse, matrix_sdk::Error> {
605601
let mut joined_room_builder = JoinedRoomBuilder::new(&self.room_id);
606602
for (message_type, add_to_timeline) in text_messages {
607-
let (event_id, timeline_item) = match message_type {
603+
let f = EventFactory::new().room(&self.room_id).sender(*BOB);
604+
let (event_id, timeline_event) = match message_type {
608605
MessageType::New { id, txt } => {
609-
let f = EventFactory::new().room(&self.room_id);
610-
let event_builder = f
611-
.text_msg(txt)
612-
.event_id(&id)
613-
.sender(*BOB)
614-
.server_ts(MilliSecondsSinceUnixEpoch::now());
606+
let event_builder =
607+
f.text_msg(txt).event_id(&id).server_ts(MilliSecondsSinceUnixEpoch::now());
615608
(id, event_builder.into_timeline())
616609
}
617610
MessageType::Edit { id, old_id, txt } => {
618-
let f = EventFactory::new().room(&self.room_id);
619611
let event_builder = f
620612
.replacement_msg(txt, &old_id)
621613
.event_id(&id)
@@ -624,7 +616,6 @@ impl TestHelper {
624616
(id, event_builder.into_timeline())
625617
}
626618
MessageType::Redaction { id, old_id } => {
627-
let f = EventFactory::new().room(&self.room_id);
628619
let event_builder = f
629620
.redaction(&old_id)
630621
.event_id(&id)
@@ -639,7 +630,7 @@ impl TestHelper {
639630
&event_id,
640631
None,
641632
Vec::new(),
642-
timeline_item.clone(),
633+
timeline_event.clone(),
643634
Vec::new(),
644635
None,
645636
Vec::new(),
@@ -648,7 +639,7 @@ impl TestHelper {
648639

649640
if add_to_timeline {
650641
joined_room_builder =
651-
joined_room_builder.add_timeline_event(timeline_item.event.cast());
642+
joined_room_builder.add_timeline_event(timeline_event.event.cast());
652643
}
653644
}
654645

0 commit comments

Comments
 (0)