Open Bug 683286 Opened 9 years ago Updated 2 years ago

Setting the src attribute for an Image causes us to decode right away even if it is not visible

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set

Tracking

()

People

(Reporter: jrmuizel, Assigned: jrmuizel, NeedInfo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 1 obsolete file)

None of the other browsers seem to do this and it can cause us to use a lot more memory.
Blocks: image-suck
Summary: Setting the src attribute for an Image causes us to decode right away → Setting the src attribute for an Image causes us to decode right away even if it is not visible
Depends on: 684091
Can we take the patch here?  What else needs to be done?
We can confirm this is an issue, especially for web applications which do pre-loading.

On our game (cf http://cardstories.org/), visitors go through a loading screen, which pre-loads several large sprites using Image objects. On Firefox post-4.0 we see a pretty huge spike of memory consumption of ~250MB during the preloading, compared to 20-25MB on other browsers, or previous versions of Firefox.

The memory is freed pretty quickly - the garbage collector seem to properly clean things up right after, but in the meantime the uncompressed images heavily bloat the memory, creating crashes/slowdowns for players who use Firefox. 

I see there is a patch for this issue, but I'm unsure about the status as it's sitting there quietly since September. Any chance to get it merged for Firefox 11? :D
I'm attaching a quick benchmark of the memory usage for 3 different versions: Firefox 4.0-b13pre, Firefox 8.0, Firefox nightly build (9/12). 

Details of the issue and ODS of the results available at http://tickets.farsides.com/issues/528#note-17
Comment on attachment 557495 [details] [diff] [review]
Don't request a decode when we don't need it

Joe, I choose you!
Attachment #557495 - Flags: review?(joe)
Comment on attachment 557495 [details] [diff] [review]
Don't request a decode when we don't need it

Review of attachment 557495 [details] [diff] [review]:
-----------------------------------------------------------------

Josh uses JOE! It's super effective!
Attachment #557495 - Flags: review?(joe) → review+
Jeff and I agree that we should put this in at the beginning of the next cycle.
Whiteboard: [MemShrink]
Jeff, can you land this as soon as you're back from holidays?  Thanks.
Whiteboard: [MemShrink] → [MemShrink:P2]
I'd like to write a test for this before landing, and need to think about the implications of this, i.e. will it keep us from firing onload?
Assignee: nobody → jmuizelaar
(In reply to Xavier Antoviaque from comment #3)
...
> The memory is freed pretty quickly - the garbage collector seem to properly
> clean things up right after, but in the meantime the uncompressed images
> heavily bloat the memory, creating crashes/slowdowns for players who use
> Firefox. 
> 

Can someone confirm the memory is released correctly when using something like new Image().src = url" for preloading? I worry that bug 661304 implies that memory will not be freed if the JavaScript calling it is in the context of the current tab (even though the image object is orphaned its part of the current tab)?

In the meantime is there a workaround that will help like explicitly setting the src to null after the image is loaded or reusing Image objects?
> Can someone confirm the memory is released correctly when using something like new Image().src = 
> url" for preloading?

You can check pretty easily using about:memory.

> I worry that bug 661304 implies that memory will not be freed if the JavaScript calling it is in the 
> context of the current tab (even though the image object is orphaned its part of the current tab)?

That should only apply to images which are in the document.
(In reply to Justin Lebar [:jlebar] from comment #11)
> > I worry that bug 661304 implies that memory will not be freed if the JavaScript calling it is in the 
> > context of the current tab (even though the image object is orphaned its part of the current tab)?
> 
> That should only apply to images which are in the document.

Why do you think that?  Bug 683290 is still open ...
> Why do you think that?  Bug 683290 is still open ...

Ah, right.  We really should fix that...
(In reply to Justin Lebar [:jlebar] from comment #13)
> > Why do you think that?  Bug 683290 is still open ...
> 
> Ah, right.  We really should fix that...

I'm working on it!  Hopefully some more movement on the dependencies next week.
Jeff, what's the status of this patch? Did you have a chance to think about its side-effects? I'd really like to have this for bug 650968.
(In reply to Tim Taubert [:ttaubert] from comment #15)
> Jeff, what's the status of this patch? Did you have a chance to think about
> its side-effects? I'd really like to have this for bug 650968.

No, I haven't had a chance to look at this anymore since I wrote last.
Comment on attachment 557495 [details] [diff] [review]
Don't request a decode when we don't need it

This code no longer exists.
Attachment #557495 - Attachment is obsolete: true
Jeff should we just close this bug? It seems like we did a big refactor of image loading since filing.
Flags: needinfo?(jmuizelaar)
You need to log in before you can comment on or make changes to this bug.