Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sdk): Various tiny improvements for LinkedChunk #3236

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Mar 19, 2024

Those patches have been cherry-picked from #3230. It's a collection of tiny improvements or bug fixes for LinkedChunk. More have been added after that. It's best to review them one by one.


Hywan added 8 commits March 19, 2024 09:37
This patch is about an internal thing, but it makes the code easier
to understand.
This patch implements the new `LinkedChunk::items` method that returns
a forward iterator over items.
This patch implements `From<ChunkIdentifier>` for `ItemPosition`.
It's useful when we get a `ChunkIdentifier` and we need to `insert_…
_at(item_position)`.
This patch implements `Chunk::content` to get an immutable reference to
the content of a chunk.
This patch implements `Copy` and `Clone` for `ItemPosition`.
This patch implements the `LinkedChunk::chunks` method that returns a
forward iterator over the chunks.
This patch adds the new `Chunk::as_ptr` method. It simplifies the code a
little bit.
@Hywan Hywan requested a review from bnjbvr March 19, 2024 08:40
@Hywan Hywan requested a review from a team as a code owner March 19, 2024 08:40
@Hywan Hywan removed the request for review from a team March 19, 2024 08:41
Hywan added 2 commits March 19, 2024 09:48
This patch rewrites `LinkedChunk::replace_gap_at`. Instead of inserting
new items on the _previous_ chunk of the gap and doing all the stuff
here, a new items chunk is created _after_ the gap (where items are
pushed), to finally unlink and remove the gap.

First off, it removes an `unwrap`. Second, if the previous chunk was
an items chunk, and had free space, the items would have been added in
there, which is not the intended behaviour. The tests have been updated
accordingly.
@Hywan Hywan force-pushed the feat-sdk-event-cache-linked-chunk-various-improvements branch from 8b2ff8a to 06e212c Compare March 19, 2024 08:48
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 82.75862% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 83.66%. Comparing base (b2c96b7) to head (e264482).
Report is 18 commits behind head on main.

Files Patch % Lines
crates/matrix-sdk/src/event_cache/linked_chunk.rs 85.71% 8 Missing ⚠️
crates/matrix-sdk/src/event_cache/store.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3236   +/-   ##
=======================================
  Coverage   83.66%   83.66%           
=======================================
  Files         236      236           
  Lines       24402    24429   +27     
=======================================
+ Hits        20416    20439   +23     
- Misses       3986     3990    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hywan added 3 commits March 19, 2024 10:05
This patch implements `Chunk::first_item_position` and
`Chunk::last_item_position` as a way to _position_ the
“cursor” (`ItemPosition`) in the correct way when we want to do some
particular insertion (at the beginning or at the end of a chunk).
This patch optimises `LinkedChunk::rchunks` and `chunks` by _not_ using
`rchunks_from` and `chunks_from`. Indeed, it's faster to not call the
inner `chunk` method + `unwrap`ping the result and so on. Just a tiny
optimisation.

This patch also uses the new `Chunk::last_item_position` method for
`LinkedChunk::items`. Abstraction for the win!
Previously, in a chunk with items `a`, `b` and `c`, their indices were
2, 1 and 0. It creates a problem when we push new items: all indices
must be shifted to the left inside the same chunk. That's not optimised.
This patch reverses the order, thus now `a` has index 0, `b` has index 1
and `c` has index 2.

It also removes a possible bug where we use `item_index` without
“reversing” it. This is now obvious that we don't need to re-compute the
`item_index`, we can use it directly.
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat!

crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
The “position” can be placed inside a `Gap`, so naming it `Item…` is a
non-sense. This patch removes the `Item` prefix.
@Hywan Hywan enabled auto-merge March 19, 2024 19:24
@Hywan Hywan merged commit fa5ce1d into matrix-org:main Mar 19, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants