GeopJr / Tuba

Browse the Fediverse
https://tuba.geopjr.dev/
GNU General Public License v3.0
503 stars 55 forks source link

[Bug]: Custom mastodon emojis will re-downloaded every time a new post is made #954

Closed fabiscafe closed 1 week ago

fabiscafe commented 1 month ago

Describe the bug

The full set of custom emoji will be re-downloaded every time you open a new post and click the custom emoji-picker button. Resulting in high traffic over eventually small time periods.

Steps To Reproduce

  1. Open Tuba
  2. CTRL+T (create new post)
  3. click on custom emoji picker
  4. go back to your timeline (close new post)
  5. CTRL+T (create new post)
  6. click on custom emoji picker ^ In this exacmple you will have the full set of custom emoji redownloaded 2 times

Logs and/or Screenshots

This is Tuba on mstdn.social, the first time opening the emoji picker grafik

This is Tuba on mstdn.social opening the picker 7 times grafik

This is the tuba log while opening the composer, click on the emoji picker, closing the composer and open it again, clicking on the emoji picker tuba.log

Instance Backend

Mastodon

Operating System

Arch Linux

Package

OS repositories

Troubleshooting information

os: Arch Linux (null) prefix: /usr flatpak: false version: 0.7.2 (production) gtk: 4.14.4 (4.14.4) libadwaita: 1.5.0 (1.5.0) libsoup: 3.4.4 (3.4.4) libgtksourceview: 5.12.0 (5.12.0)

Additional Context

This might however not be a tuba-related issue, but mstdn.social or mastodon itself. See https://mstdn.social/@fabiscafe/112501660155684610. In there I was able to recreate this via firefox and mstdn.social-web. However the load was only half of what it is on Tuba.

The command to catch network traffic is

# nethogs -v3
GeopJr commented 1 month ago

For starters, I see a leak tbh. Emojis should (and are) being disposed, along with the custom emoji picker, when you close the composer. Something somewhere isn't getting disposed and I'll have to investigate it...

For Mastodon-web:

Using the inspector, I see that the images are in fact cached. One thing Mastodon-web does that tuba doesn't, is that it loads only what's visible. Idk if mstdn.social has a lot of emojis but on tech.lgbt, which has wayyyyyyy too many, it will load whatever's visible and load more as I scroll. Scrolling back up hits the cache according to the inspector.

So, while Tuba might show 100MB received but mstdn 10MB, it's probably because Tuba loaded all the emojis, while mastodon-web only a portion.

For Tuba:

Cache depends 100% on libsoup and I'm not that satisfied with it since nethogs shows that it's not hitting it. I'll see about bringing back in-house in-memory cache for all images.

For now, I did actually implement your suggestion last time you brought it up on matrix but haven't merged it yet: #915

Why bring back in-memory cache?

I've been experimenting with ListViews (again, like I do every month, 'Insanity is doing the same thing over and over again and expecting different results.' etc etc etc). One major bottle neck is decoding images. It's blocking but due to the amount of images that might load at a time (emojis, pfp, attachments), threading is unstable.

Keeping an in-memory cache, the one from Tootle's time that had reference counting, will allow us to re-use paintables rather than decoding again and again and again...

That's what libsoup's cache does. It caches responses, not the final object (as expected). So when you GET https://.../avatar.png, it will return the cached file, which still needs to be decoded.

fabiscafe commented 1 month ago

For starters, I see a leak tbh. Emojis should (and are) being disposed, along with the custom emoji picker, when you close the composer. Something somewhere isn't getting disposed and I'll have to investigate it...

Does this mean the current behavior is actually wanted to some point? Because I also have this on the official app and sadly are often in areas with SLOW data connection. So I have to wait for Emoji to load for > 30 sec sometimes and this is just a horrible design choice.

For Mastodon-web:

Using the inspector, I see that the images are in fact cached. One thing Mastodon-web does that tuba doesn't, is that it loads only what's visible.

Yep I have scrolled down until all were loaded (But obviously some must still not be there). I was just shocked that it would load and load them again with every new tab I use, or by just hitting F5 to reload.

For now, I did actually implement your suggestion last time you brought it up on matrix but haven't merged it yet: https://github.com/GeopJr/Tuba/pull/915

I just applied it for testing. It looks like it does work fine for me. Only one massive download the first time and the waiting time for every following composer opening is nicely short even on the slowest system I own. Do you plan to merge this at some point or is this just for testing purposes?

--

For the in-memory cache, what would be the benefit compared with #915? I guess it's faster?

GeopJr commented 1 month ago

Does this mean the current behavior is actually wanted to some point? Because I also have this on the official app and sadly are often in areas with SLOW data connection. So I have to wait for Emoji to load for > 30 sec sometimes and this is just a horrible design choice.

The re-downloading part is not being done on purpose. libsoup is supposed to be keeping track of cache... but apparently cache doesn't hit for some unknown reason on emojis :shrug: I'll investigate it but it's not something 'by design'.

The design was:

Yep I have scrolled down until all were loaded (But obviously some must still not be there). I was just shocked that it would load and load them again with every new tab I use, or by just hitting F5 to reload.

Here's let's take a deeper look,

Opening the emoji picker on elk.zone and switching to the custom emojis section:

image

Notice the straight forward and expected 'cached' under transferred! We can assume caching works right (i havent done network logging but im choosing to trust firefox).

Between refreshes, some emojis are being raced (both a network and a cache request, network request was faster), some are straight from cache.

Now let's take a look on mastodon.social:

image

Why does it use a service worker? Does it cache images properly? It claims that the cache was hit, but did it?

:shrug:

I just applied it for testing. It looks like it does work fine for me. Only one massive download the first time and the waiting time for every following composer opening is nicely short even on the slowest system I own. Do you plan to merge this at some point or is this just for testing purposes?

For the in-memory cache, what would be the benefit compared with https://github.com/GeopJr/Tuba/pull/915? I guess it's faster?

Yep will merge it for the next release.

Ideally, using both #915 and in-memory cache would be the best.

915 still needs to handle the images, but a lot less processing because it reads the file directly. In-memory cache goes one step further by keeping the result object (after processing) in a centralized place in memory, so when for example, you have a post with the same emoji twice, it will use the same instance of the result object for both, instead of creating two different ones (and using twice the memory).

I have it ready locally, just doing a bit more testing before pushing

Honestly, I want to optimize the custom emoji picker by using a GridView instead - which will have the same effect as mastodon-web; it will only load some of them and load/unload as you scroll. But... all *View containers are so limiting that I'll have to overengineer it :(

GeopJr commented 1 week ago

I'm closing this as fixed by #915. Abstract cache (#957) requires more work (to be 100% optimized and avoid not reusing paintables) and the issue is technically fixed by #915!

Hopefully I can land it in a patch if it has a positive impact otherwise I'll drop it

edit:

Also, I'll add that the leak is a deeper issue with a dependency (gtk, gdk etc). I did spend time debugging and sysprofing it. From Tuba's side it gets completely disposed, in fact I made sure to force every single paintable, emoji, widget to be disposed for the sake of making sure. I'll focus on moving to a recycler view, one way or another in an attempt to counter it. FWIW, it only happens when there are wayyyyyy too many emojis aka both mstdn.social and tech.lgbt... Please use the custom emoji autocompletion instead for the time being

edit2:

Which, I mean, I can't blame any internal part for breaking, 3k images is a lot...