Skip to content

Commit 77918af

Browse files
HywanHywan
committed
doc+test(sdk): Add documentation for AsVectorSubscriber + improve tests.
This patch add more documentation for `AsVectorSubscriber` and improve the tests.
1 parent e406f1f commit 77918af

File tree

2 files changed

+258
-36
lines changed

2 files changed

+258
-36
lines changed

‎crates/matrix-sdk/src/event_cache/linked_chunk/as_vector.rs

Lines changed: 253 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -28,20 +28,38 @@ use super::{
2828
ChunkIdentifier,
2929
};
3030

31-
type Offset = usize;
31+
/// A type alias to represent a chunk's length. This is purely for commodity.
3232
type ChunkLength = usize;
3333

3434
pin_project! {
35+
/// A type used to transform a `Stream<Item = Vec<Update<Item, Gap>>>` into
36+
/// a `Stream<Item = Vec<VectorDiff<Item>>>`. Basically, it helps to consume
37+
// a [`LinkedChunk<CAP, Item, Gap>`] as if it was an [`ObservableVector<Item>`].
3538
pub struct AsVectorSubscriber<Item, Gap> {
39+
// The inner `UpdatesSubscriber`.
3640
#[pin]
3741
updates_subscriber: UpdatesSubscriber<Item, Gap>,
3842

43+
// Pairs of all known chunks and their respective length. This is the only
44+
// required data for this algorithm.
3945
chunks: VecDeque<(ChunkIdentifier, ChunkLength)>,
4046
}
4147
}
4248

4349
impl<Item, Gap> AsVectorSubscriber<Item, Gap> {
44-
pub(super) fn new(
50+
/// Create a new [`Self`].
51+
///
52+
/// `updates_subscriber` is simply `UpdatesSubscriber`.
53+
/// `initial_chunk_lengths` must be pairs of all chunk identifiers with the
54+
/// associated chunk length. The pairs must be in the exact same order than
55+
/// in [`LinkedChunk`].
56+
///
57+
/// # Safety
58+
///
59+
/// This method is marked as unsafe only to attract the caller's attention
60+
/// on the order of pairs. If the order of pairs are incorrect, the entire
61+
/// algorithm will not work properly and is very likely to panic.
62+
pub(super) unsafe fn new(
4563
updates_subscriber: UpdatesSubscriber<Item, Gap>,
4664
initial_chunk_lengths: VecDeque<(ChunkIdentifier, ChunkLength)>,
4765
) -> Self {
@@ -65,39 +83,138 @@ where
6583

6684
let mut diffs = Vec::with_capacity(updates.len());
6785

68-
let mut reattaching = false;
86+
// A flag specifying when updates are reattaching detached items.
87+
//
88+
// Why is it useful?
89+
//
90+
// Imagine a `LinkedChunk::<3, char, ()>` containing `['a', 'b', 'c'] ['d']`. If
91+
// one wants to insert [`w`, x`, 'y', 'z'] at position
92+
// `Position(ChunkIdentifier(0), 1)`, i.e. at the position of `b`, here is what
93+
// happens:
94+
//
95+
// 1. `LinkedChunk` will split off `['a', 'b', 'c']` at index 1, the chunk
96+
// becomes `['a']` and `b` and `c` are _detached_, thus we have:
97+
//
98+
// ['a'] ['d']
99+
//
100+
// 2. `LinkedChunk` will then insert `w`, `x`, `y` and `z` to get:
101+
//
102+
// ['a', 'w', 'x'] ['y', 'z'] ['d']
103+
//
104+
// 3. `LinkedChunk` will now reattach `b` and `c` after `z`, like so:
105+
//
106+
// ['a', 'w', 'x'] ['y', 'z', 'b'] ['c'] ['d']
107+
//
108+
// This detaching/reattaching approach makes it reliable and safe. Good. Now,
109+
// what updates are we going to receive for each step?
110+
//
111+
// Step 1, detaching last items:
112+
//
113+
// ```
114+
// Update::DetachLastItems { at: Position(ChunkIdentifier(0), 1) }
115+
// ```
116+
//
117+
// Step 2, inserting new items:
118+
//
119+
// ```
120+
// Update::PushItems {
121+
// position_hint: Position(ChunkIdentifier(0), 1),
122+
// items: vec!['w', 'x'],
123+
// }
124+
// Update::NewItemsChunk {
125+
// previous: Some(ChunkIdentifier(0)),
126+
// new: ChunkIdentifier(2),
127+
// next: Some(ChunkIdentifier(1)),
128+
// }
129+
// Update::PushItems {
130+
// position_hint: Position(ChunkIdentifier(2), 0),
131+
// items: vec!['y', 'z'],
132+
// }
133+
// ```
134+
//
135+
// Step 3, reattaching detached items:
136+
//
137+
// ```
138+
// Update::ReattachItems
139+
// Update::PushItems {
140+
// position_hint: Position(ChunkIdentifier(2), 2),
141+
// items: vec!['b']
142+
// }
143+
// Update::NewItemsChunk {
144+
// previous: Some(ChunkIdentifier(2)),
145+
// new: ChunkIdentifier(3),
146+
// next: Some(ChunkIdentifier(1)),
147+
// }
148+
// Update::PushItems {
149+
// position_hint: Position(ChunkIdentifier(3), 0),
150+
// items: vec!['c'],
151+
// }
152+
// Update::ReattachItemsDone
153+
// ```
154+
//
155+
// To ensure an optimised behaviour of this algorithm:
156+
//
157+
// * `Update::DetachLastItems` must not emit `VectorDiff::Remove`,
158+
//
159+
// * `Update::PushItems` must not emit `VectorDiff::Insert`s or
160+
// `VectorDiff::Append`s if it happens after `ReattachItems` and before
161+
// `ReattachItemsDone`. However, `Self::chunks` must always be updated.
162+
//
163+
// From the `VectorDiff` “point of view”, this optimisation aims at avoiding
164+
// removing items to push them again later.
165+
let mut mute_push_items = false;
69166

70167
for update in updates {
71168
match update {
72169
Update::NewItemsChunk { previous, new, next }
73170
| Update::NewGapChunk { previous, new, next, .. } => {
74171
match (previous, next) {
75172
// New chunk at the end.
76-
(Some(_previous), None) => {
77-
// TODO: chec `previous` is correct
173+
(Some(previous), None) => {
174+
debug_assert!(
175+
matches!(this.chunks.back(), Some((p, _)) if *p == previous),
176+
"Inserting new chunk at the end: The previous chunk is invalid"
177+
);
178+
78179
this.chunks.push_back((new, 0));
79180
}
80181

81182
// New chunk at the beginning.
82-
(None, Some(_next)) => {
83-
// TODO: check `next` is correct
183+
(None, Some(next)) => {
184+
debug_assert!(
185+
matches!(this.chunks.front(), Some((n, _)) if *n == next),
186+
"Inserting new chunk at the end: The previous chunk is invalid"
187+
);
188+
84189
this.chunks.push_front((new, 0));
85190
}
86191

87192
// New chunk is inserted between 2 chunks.
88-
(Some(_previous), Some(next)) => {
89-
// TODO: check `previous` is correct
193+
(Some(previous), Some(next)) => {
90194
let next_chunk_index = this
91195
.chunks
92196
.iter()
93197
.position(|(chunk_identifier, _)| *chunk_identifier == next)
94-
.expect("next chunk not found");
198+
// SAFETY: Assuming `LinkedChunk` and `Updates` are not buggy, and
199+
// assuming `Self::chunks` is correctly initialized, it is not
200+
// possible to insert a chunk between two chunks where one does not
201+
// exist. If this predicate fails, it means `LinkedChunk` or
202+
// `Updates` contain a bug.
203+
.expect("Inserting new chunk: The chunk is not found");
204+
205+
debug_assert!(
206+
matches!(this.chunks.get(next_chunk_index - 1), Some((p, _)) if *p == previous),
207+
"Inserting new chunk: The previous chunk is invalid"
208+
);
95209

96210
this.chunks.insert(next_chunk_index, (new, 0));
97211
}
98212

99213
(None, None) => {
100-
unreachable!("?");
214+
unreachable!(
215+
"Inserting new chunk with no previous nor next chunk identifiers \
216+
is impossible"
217+
);
101218
}
102219
}
103220
}
@@ -109,9 +226,15 @@ where
109226
.position(|(chunk_identifier, _)| {
110227
*chunk_identifier == expected_chunk_identifier
111228
})
112-
.expect("oops 4");
113-
114-
this.chunks.remove(chunk_index).unwrap();
229+
// SAFETY: Assuming `LinkedChunk` and `Updates` are not buggy, and assuming
230+
// `Self::chunks` is correctly initialized, it is not possible to remove a
231+
// chunk that does not exist. If this predicate fails, it means
232+
// `LinkedChunk` or `Updates` contain a bug.
233+
.expect("Removing a chunk: The chunk is not found");
234+
235+
// It's OK to ignore the result. The `chunk_index` exists because it's been
236+
// found, and we don't care about its associated value.
237+
let _ = this.chunks.remove(chunk_index);
115238
}
116239

117240
Update::PushItems { position_hint: position, items } => {
@@ -134,11 +257,16 @@ where
134257
ControlFlow::Continue(..) => None,
135258
}
136259
}
137-
.expect("`ChunkIdentifier` must exist");
260+
// SAFETY: Assuming `LinkedChunk` and `Updates` are not buggy, and assuming
261+
// `Self::chunks` is correctly initialized, it is not possible to push items on
262+
// a chunk that does not exist. If this predicate fails, it means `LinkedChunk`
263+
// or `Updates` contain a bug.
264+
.expect("Pushing items: The chunk is not found");
138265

139266
*chunk_length += items.len();
140267

141-
if reattaching {
268+
// See `mute_push_items` to learn more.
269+
if mute_push_items {
142270
continue;
143271
}
144272

@@ -164,17 +292,23 @@ where
164292
.find_map(|(chunk_identifier, length)| {
165293
(*chunk_identifier == expected_chunk_identifier).then(|| length)
166294
})
167-
.expect("oops 3");
295+
// SAFETY: Assuming `LinkedChunk` and `Updates` are not buggy, and assuming
296+
// `Self::chunks` is correctly initialized, it is not possible to detach
297+
// items from a chunk that does not exist. If this predicate fails, it means
298+
// `LinkedChunk` or `Updates` contain a bug.
299+
.expect("Detach last items: The chunk is not found");
168300

169301
*length = new_length;
170302
}
171303

172304
Update::ReattachItems => {
173-
reattaching = true;
305+
// Entering the `reattaching` mode.
306+
mute_push_items = true;
174307
}
175308

176309
Update::ReattachItemsDone => {
177-
reattaching = false;
310+
// Exiting the `reattaching` mode.
311+
mute_push_items = false;
178312
}
179313
}
180314
}
@@ -199,40 +333,126 @@ mod tests {
199333

200334
assert_pending!(as_vector);
201335

202-
linked_chunk.push_items_back(['a', 'b']);
203-
linked_chunk.push_items_back(['c', 'd', 'e']);
204-
336+
linked_chunk.push_items_back(['a', 'b', 'c', 'd']);
337+
#[rustfmt::skip]
338+
assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['d']);
339+
340+
// From an `ObservableVector` point of view, it would look like:
341+
//
342+
// 0 1 2 3 4
343+
// +---+---+---+---+
344+
// | a | b | c | d |
345+
// +---+---+---+---+
346+
// ^^^^^^^^^^^^^^^^
347+
// |
348+
// new
205349
assert_next_eq!(
206350
as_vector,
207351
&[
208-
VectorDiff::Append { values: vector!['a', 'b'] },
209-
VectorDiff::Append { values: vector!['c'] },
210-
VectorDiff::Append { values: vector!['d', 'e'] },
352+
VectorDiff::Append { values: vector!['a', 'b', 'c'] },
353+
VectorDiff::Append { values: vector!['d'] },
211354
]
212355
);
213356

214357
linked_chunk
215-
.insert_items_at(['f', 'g'], linked_chunk.item_position(|item| *item == 'b').unwrap())
358+
.insert_items_at(
359+
['w', 'x', 'y', 'z'],
360+
linked_chunk.item_position(|item| *item == 'b').unwrap(),
361+
)
216362
.unwrap();
217-
363+
assert_items_eq!(linked_chunk, ['a', 'w', 'x'] ['y', 'z', 'b'] ['c'] ['d']);
364+
365+
// From an `ObservableVector` point of view, it would look like:
366+
//
367+
// 0 1 2 3 4 5 6 7 8
368+
// +---+---+---+---+---+---+---+---+
369+
// | a | w | x | y | z | b | c | d |
370+
// +---+---+---+---+---+---+---+---+
371+
// ^^^^^^^^^^^^^^^^
372+
// |
373+
// new
218374
assert_next_eq!(
219375
as_vector,
220376
&[
221-
VectorDiff::Insert { index: 1, value: 'f' },
222-
VectorDiff::Insert { index: 2, value: 'g' },
377+
VectorDiff::Insert { index: 1, value: 'w' },
378+
VectorDiff::Insert { index: 2, value: 'x' },
379+
VectorDiff::Insert { index: 3, value: 'y' },
380+
VectorDiff::Insert { index: 4, value: 'z' },
223381
]
224382
);
225383

226384
linked_chunk.push_gap_back(());
227-
linked_chunk.push_items_back(['h', 'i']);
385+
linked_chunk.push_items_back(['e', 'f', 'g', 'h']);
386+
assert_items_eq!(
387+
linked_chunk,
388+
['a', 'w', 'x'] ['y', 'z', 'b'] ['c'] ['d'] [-] ['e', 'f', 'g'] ['h']
389+
);
228390

229-
assert_next_eq!(as_vector, &[VectorDiff::Append { values: vector!['h', 'i'] }]);
391+
// From an `ObservableVector` point of view, it would look like:
392+
//
393+
// 0 1 2 3 4 5 6 7 8 9 10 11 12
394+
// +---+---+---+---+---+---+---+---+---+---+---+---+
395+
// | a | w | x | y | z | b | c | d | e | f | g | h |
396+
// +---+---+---+---+---+---+---+---+---+---+---+---+
397+
// ^^^^^^^^^^^^^^^^
398+
// |
399+
// new
400+
assert_next_eq!(
401+
as_vector,
402+
&[
403+
VectorDiff::Append { values: vector!['e', 'f', 'g'] },
404+
VectorDiff::Append { values: vector!['h'] }
405+
]
406+
);
230407

231408
linked_chunk
232-
.replace_gap_at(['j'], linked_chunk.chunk_identifier(|chunk| chunk.is_gap()).unwrap())
409+
.replace_gap_at(
410+
['i', 'j', 'k', 'l'],
411+
linked_chunk.chunk_identifier(|chunk| chunk.is_gap()).unwrap(),
412+
)
233413
.unwrap();
414+
assert_items_eq!(
415+
linked_chunk,
416+
['a', 'w', 'x'] ['y', 'z', 'b'] ['c'] ['d'] ['i', 'j', 'k'] ['l'] ['e', 'f', 'g'] ['h']
417+
);
418+
419+
// From an `ObservableVector` point of view, it would look like:
420+
//
421+
// 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
422+
// +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
423+
// | a | w | x | y | z | b | c | d | i | j | k | l | e | f | g | h |
424+
// +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
425+
// ^^^^^^^^^^^^^^^^
426+
// |
427+
// new
428+
assert_next_eq!(
429+
as_vector,
430+
vec![
431+
VectorDiff::Insert { index: 8, value: 'i' },
432+
VectorDiff::Insert { index: 9, value: 'j' },
433+
VectorDiff::Insert { index: 10, value: 'k' },
434+
VectorDiff::Insert { index: 11, value: 'l' },
435+
]
436+
);
437+
438+
linked_chunk
439+
.insert_items_at(['m'], linked_chunk.item_position(|item| *item == 'a').unwrap())
440+
.unwrap();
441+
assert_items_eq!(
442+
linked_chunk,
443+
['m', 'a', 'w'] ['x'] ['y', 'z', 'b'] ['c'] ['d'] ['i', 'j', 'k'] ['l'] ['e', 'f', 'g'] ['h']
444+
);
234445

235-
assert_next_eq!(as_vector, &[VectorDiff::Insert { index: 7, value: 'j' }]);
446+
// From an `ObservableVector` point of view, it would look like:
447+
//
448+
// 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17
449+
// +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
450+
// | m | a | w | x | y | z | b | c | d | i | j | k | l | e | f | g | h |
451+
// +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
452+
// ^^^^
453+
// |
454+
// new
455+
assert_next_eq!(as_vector, vec![VectorDiff::Insert { index: 0, value: 'm' }]);
236456

237457
drop(linked_chunk);
238458
assert_closed!(as_vector);

0 commit comments

Comments
 (0)