matrix-rust-sdk icon indicating copy to clipboard operation
matrix-rust-sdk copied to clipboard

feat(send_queue): report progress for media uploads

Open Johennes opened this issue 7 months ago • 4 comments

This makes it possible to listen to the media upload progress when using the send queue. The progress is communicated via EventSendState::NotSentYet.

I haven't yet fixed / enhanced the tests or added a changelog because I'm not 100% sure about this approach and would like to get some preliminary feedback first.

  • [ ] Public API changes documented in changelogs (optional)

  • To see the specific tasks where the Asana app for GitHub is being used, see below:
    • https://app.asana.com/0/0/1210187425391164

Johennes avatar May 06 '25 18:05 Johennes

I just tested this on EXA and it seems like there's a memory peak when receiving the upload progress:

https://github.com/user-attachments/assets/7dc8a55e-4237-4c12-a10e-ed4d72c4c710

Keep in mind though that this may also be caused by the upload process itself.


I tried measuring the allocations now and I got this:

image

EventSendProgress$MediaUpload is the binding version of the SDK record, MediaUploadProgress is what we map it to. I think we have 2x the SDK items because we don't map them in the pinned events timeline, so they're discarded.

The range in the image shows an spike in general allocations, specially in the Java Heap. It's a shame we can't filter out items for the graph and just get the values for these allocations, we have no easy way of knowing whether these allocations come mainly from the EventSendProgress objects and their mapping process or there's something else happening at the same time.

jmartinesp avatar May 28 '25 07:05 jmartinesp

@jmartinesp Thanks! Can you measure if the memory peak maps to the size of the media, or is it something else?

Hywan avatar May 28 '25 09:05 Hywan

image

I'd say it looks quite similar without these changes, the image above being a bit more stretched.

jmartinesp avatar May 28 '25 10:05 jmartinesp

@jmartinesp thanks a bunch for taking the time to test the impact of this on Android! ❤️

Johennes avatar May 28 '25 10:05 Johennes

Codecov Report

:x: Patch coverage is 90.21277% with 23 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 88.80%. Comparing base (18b169c) to head (7cba439). :warning: Report is 130 commits behind head on main. :white_check_mark: All tests successful. No failed tests found.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/send_queue/mod.rs 91.34% 17 Missing and 1 partial :warning:
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs 64.28% 5 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5008      +/-   ##
==========================================
+ Coverage   88.78%   88.80%   +0.01%     
==========================================
  Files         334      334              
  Lines       91236    91414     +178     
  Branches    91236    91414     +178     
==========================================
+ Hits        81007    81178     +171     
- Misses       6400     6406       +6     
- Partials     3829     3830       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jul 08 '25 10:07 codecov[bot]

@bnjbvr this is now ready for review again. I've addressed all previous comments other than https://github.com/matrix-org/matrix-rust-sdk/pull/5008#discussion_r2190753736. Since I had previously made a mess out of the git history I also rebased everything into 10 self-contained commits. I will use only fix-ups from here on but wanted to try and make the base as easy to review as possible.

Regarding the approach, I went for a mix of using the event cache, info from dependent requests and an in-memory cache for thumbnail sizes.

Johennes avatar Jul 08 '25 10:07 Johennes

Usually at this point, I'd try to help with the merging of this PR, by addressing the comments myself, but turns out I'll be away starting today and for a week or so, so I don't have much time to do this. If you're bored with this PR, I'll be happy to address my own comments when I'm back. Alternatively, another reviewer can make sure that my comments have been addressed in the meanwhile (although it'd be a bit of a stretch for them to read all the prior work).

Thanks for the review. 🙏 Letting this sit until you return seems fine from my perspective. I have addressed all comments apart from those I left unresolved.

PS: There was a conflict from a change on main. I wasn't sure how to resolve this but the folks in #matrix-rust-sdk-dev:flipdot.org said a merge should be ok.

Johennes avatar Jul 10 '25 13:07 Johennes

I've posted a few more comments below, but does it sound reasonable to break the PR apart in smaller chunks again, please?

I have opened #5453 for the send queue part and #5454 for the timeline part (which will include the send queue part until #5453 lands). I'll close this to retain your comments that still need to be addressed.

Johennes avatar Jul 28 '25 12:07 Johennes