matrix-rust-sdk
matrix-rust-sdk copied to clipboard
feat(send_queue): report progress for media uploads
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
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:
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 Thanks! Can you measure if the memory peak maps to the size of the media, or is it something else?
I'd say it looks quite similar without these changes, the image above being a bit more stretched.
@jmartinesp thanks a bunch for taking the time to test the impact of this on Android! ❤️
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.
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.
@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.
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.
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.