While displaying MMS thumbnails with blob we can revoke it after it's not needed anymore.

In my oppinion, revoke is in the correct place so I've just changed from using toDataUrl to blob.

Comment on attachment 772624 [details] first version of patch will review

Comment on attachment 772624 [details] first version of patch see review on github thanks for this work, this is not an easy task !

just filed bug 891884 because I discovered that toBlob does not support the quality argument yet.

Changed according to previous gh comments (https://github.com/mozilla-b2g/gaia/pull/10863). Do we wait for #891884 ?

I would have liked to assess the performance and memory impact of this patch like I did for bug 889899. Do we expect significant improvements here or is this just a "nice to have" change?

I think we expect a little bit of memory savings here Gabriele

Comment on attachment 775770 [details] updated patch Gabe - Could you take a look at the memory usage of a thread before and after with this patch? We aren't expecting anything huge, but there should be at least a little bit of performance to be gained here.

I've tested the PR against the current gaia master branch and it seems that the PR is regressing a bit from the master branch. I've tested using the database contained in attachment 776370 [details] which contains a huge thread of MMSes with images. Here's the result: * master branch time to open the thread ~55s peak memory while opening the thread ~82MiB memory consumption once the application is minimized ~34MiB * PR in attachment 775770 [details] time to open the thread ~60s peak memory while opening the thread ~93MiB memory consumption once the application is minimized ~45MiB As you can see there's a 10MiB difference between the two and it's entirely made up of images (I've taken a memory dump with about:memory forcing minimization on both tests). To be frank I don't know why this is happening; the about:memory dump from the unmodified gaia branch has some weird data in it (negative size of certain DOM elements!?) but the RSS is consistent in showing that this PR increases steady memory use by ~10MiB. I will re-test using an m-c based Gecko to see if I can gain some further insight about what's happening with the images.

This is a memory report taken after opening the test thread; to read it unpack it first, then enter the about:memory page on your desktop Firefox, click on the "Read report from file" and select the .json file you've extracted. Looking carefully at the memory report it seems that the PR is keeping around the raw data for every thumbnail it creates: 23.25 MB (100.0%) -- explicit ├──10.40 MB (44.73%) -- images │ ├──10.40 MB (44.73%) -- content │ │ ├──10.40 MB (44.73%) -- used │ │ │ ├──10.40 MB (44.72%) ── raw Previously on the other hand this data was discarded by the GC. I don't understand this code particularly well so I was wondering: are the attachment being rendered only when the containing div is visible? That would explain why the (non-visible) images are being thrown away in the master branch but not with this patch. When this patch is applied the only place where we might be holding on to the raw image's data is here, in the event listener: https://github.com/siemiatj/gaia/blob/20acc94627ba3d1d75cb5a7d48480ce079a8f208/apps/sms/js/attachment.js#L204 If that's the case a possible solution would be to create the image within bubbleEvents() instead of passing it as a parameter so that we don't keep its raw data alive in the event listener.

https://github.com/gnarf/gaia/compare/mms-preview-memory - Putting this up to maybe answer the memory issues Gabriele discovered.

Just wondering - what if the iframe hasn't finished loading ? Then we won't have the img to add listener to. But I'll guess it won't matter if the memory saving is worth it.

Updated my branch to also revoke the URL in the img inside the iframe case

As far as the memory testing goes though - this doesn't seem to be saving us much. Going to wait for julien to come back around and remind me why we wanted to use blobs :)

Since the bug 891884 has been resolved, maybe setting quality will further improve the memory allocation here.

Comment on attachment 775770 [details] updated patch I've just tested the latest version of the PR and its still retaining the image raw data somehow. I think that it's still caused by the event listener here: https://github.com/gnarf/gaia/blob/20acc94627ba3d1d75cb5a7d48480ce079a8f208/apps/sms/js/attachment.js#L204 via the chain of closures involved in the process. However it's hard to tell if that's the case or not; the only thing I'm sure of is that when I navigate back to the thread list all the images are dropped as they should be.

Comment on attachment 775770 [details] updated patch I guess Corey removed his commits. This shouldn't leak anymore. Should we add the quality already ?

Comment on attachment 775770 [details] updated patch Clearing the review flag because I'm no comms app peer :) I've retested even this patch and we're still accumulating the images somewhere; at least we might say that it's not related to the event-listener though I wonder if it's still closure-related (see this post for an example of weird closure-keep-stuff-alive problems https://groups.google.com/d/msg/mozilla.dev.gaia/5Wc2VAiB7hk/F4bFIyV0qfMJ). I think that the key to this is that we're keeping around raw uncompressed image data (not the images themselves as DOM objects) so that's what we should be trying to track down. Anyway since this is proving to be a hard nut to crack we might want to re-evaluate our approach. AFAIK we opened this bug in the hope that using toBlob() would save us memory (which sounded a reasonable assumption) but we haven't been able to reap any benefits up to this point and I would argue that the current code is fairly well behaved already in that respect. Shall we keep trying or shall we drop this? One of the benefits I could measure is that once you eliminate the accumulation we're seeing peak memory consumption is lower than the previous approach while opening the mega-thread. This is good as it will prevent other applications from being killed due to OOM and allow us to open even larger threads in the same amount of memory. This is important IMHO but should be evaluated against the other pending issues.

Could the "image accumulation" simply be that they're displayed by the browser.. which needs them anyway ?

So what's the deal here? Looks like this has stalled out?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #20) > So what's the deal here? Looks like this has stalled out? Yes, for some reasons the tries that were made using blobs ended up consuming more memory than the dataurls; the about:memory dumps showed the raw images being retained whenever we used blobs. See comment 11 and comment 18 for more details.

My wild guess is: * with a data-url as src, gecko "knows" he will be able to read the source to rerender the image if necessary, and therefore it can safely drops the decoded image * with a blob-url as src, the blob url might be invlidated when gecko will need to rerender the image, and therefore it keeps the decoded image in memory. I have no clue if that's what's really happening though. Initially this bug was a first step to do other improvements like the attachment lazy loading in bug 887196, but maybe bug 887196 should be done before ? Gabriele, you also said in comment 18 that there were some improvements after all, so maybe we should just move forward ?

The Graphics team will be in Paris next week, I'll try to get answers about this ;)

Julien - Is this still a valid bug?

Yes, I have a patch from Timothy Nikkel to help debug this but I couldn't find time yet. I can attach it here in the hope that it's not bitrotted now. (keeping NI to attach the patch)

This is a debug patch given by Timothy Nikkel. Not sure this is that useful, or that it still applies, but it can help showing the image lifecycle.

After reading this post https://groups.google.com/d/msg/mozilla.dev.b2g/81lxhZQgevM/ip3eW21KAU0J I'm starting to wonder if using blobs is the right approach here. What I found in comment 10 was that when we used blobs we were actually holding to a copy of the raw image data while the same wasn't happening with dataurls. In the post Kyle mentions that data URLs are special in that the code using them knows that the underlying data is always available while it's not the case for blobs which will cause a copy of the data to be kept around. So I think that the question here is the following: are we not revoking the blobs' URLs correctly or is just impossible to drop that extra copy of the images while using blobs? If the issue is caused by how we revoke the blob URLs then the approach here might still be valid, if it's not then I fear we should just settle with data URLs and close this as WONTFIX.

We'll change this to using background-size soon anyway (bug 995116) so I expect this bug to become obsolete.

(In reply to Gabriele Svelto [:gsvelto] from comment #27) > After reading this post > https://groups.google.com/d/msg/mozilla.dev.b2g/81lxhZQgevM/ip3eW21KAU0J I'm > starting to wonder if using blobs is the right approach here. > > What I found in comment 10 was that when we used blobs we were actually > holding to a copy of the raw image data while the same wasn't happening with > dataurls. In the post Kyle mentions that data URLs are special in that the > code using them knows that the underlying data is always available while > it's not the case for blobs which will cause a copy of the data to be kept > around. That's not what I read here: For certain protocols it is guaranteed to be (e.g. data) and for others it is not (e.g. http), but we generally assume it is not since a non-idempotent scheme (http) is by far the most dominant protocol on the web. He says "we generally assume it is not..." that means to me that gecko has the same behavior for all protocols. That's also not what bug 996512 implies... My comment 22 was exactly what you think but I now think it's wrong, given what Kyle and others have said.

OK, well the strange part of this is that what we're retaining is the raw image data, not the decoded one. So in the first case we have the usage from the data URLs and in the second ones the data URLs + the raw data. I guess this means that we're not revoking the URLs correctly or there's something in the closure that's retaining the URL someway, somehow. Anyway good to know that this will be superseded by bug 995116; shall we just WONTFIX this one then?

Yeah, I'll even dupe it. Next step is probably ensure that all blobs that we need to keep for some time are from a DB or a File, and not in-memory blobs.