Stuart Parmenter Assignee
	Description • 17 years ago

I whipped up this patch to recompress images that haven't been accessed recently (i.e. ones being held on to by the bfcache or background tabs or even foreground tabs sitting out of site/use).  On average I'm seeing roughly 30% savings in overall footprint doing this.  We should look at what level of compression to use and if zlib is the best thing for the job (it doesn't do great for JPEGs).

Vlad had some suggestions on cleaning up this patch a bit but with those this should be safe to land with minimal impact.

	Stuart Parmenter Assignee
	Comment 1 • 17 years ago

I'll also note that this patch can and probably should eventually be replaced with some largish work to imagelib to keep the original image data around and rerun it through a decoder.  I don't recommend that we try to do that for Gecko 1.9.

	Jo Hermans
	Comment 2 • 17 years ago

see also bug 327280

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 3 • 17 years ago

Marking blocking because this is potentially a very big memory usage win...

I wonder if we can hack in keeping the original image data around; are images written to the memory/disk cache before they're decoded?  An alternative might be to just not keep images around in bfcache as well, or maybe getting rid of any above a certain size...

	Stuart Parmenter Assignee
	Comment 4 • 17 years ago

I looked at how to hack in keeping the data around but there isn't a simple easy hack to do it.  There are several not-super-hard-but-possibly-slightly-time-consuming ways to do it.  If we have time later on after other bugs I can make that happen.

	Jesse Ruderman
	Comment 5 • 17 years ago

I'm worried that this approach could hurt locality of reference, since it looks at image data that would otherwise be thrown away (likely for bfcache) or not accessed again for a long time (likely for background tabs).  It's probably worth it for a 30% decrease in memory use, though!

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 17 years ago

This seems to greatly reduce X memory usage.

	Jo Hermans
	Comment 7 • 17 years ago

(In reply to comment #6)
> This seems to greatly reduce X memory usage.
> 

Care to explain that ? This bug is about compressing the image-copies in Gecko, in local memory. X runs in a different process, potentially on a different host. All images in the X server are copied over the network (XCreateImage/XPutImage). We can't compress images /inside/ the X server.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 17 years ago

I don't know how our image cache works, but with the patch TB and FF use a lot
less X Server's memory.
Without patch I think FF took several MBs, with the patch ~250kB, at least 
according to Gnome system monitor. Or when loading some pages it takes few MBs but those are released soon and X mem usage comes down to 250kB.
This is how FF behaves after running it ~24h.
(Testing on Fedora7/X86_64)

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 9 • 17 years ago

So get rid of the forced PR_TRUE on shouldUseImageSurfaces, and the printfs, but other than that I can't remember what else I wanted changed... so looks fine

	Steve England [:stevee]
	Comment 10 • 17 years ago

Please forgive me if this is a stupid question, but won't compressing images in the cache mean any space saved will be used to cache other stuff, so there won't actually be any change in memory used in the long run?

	Stuart Parmenter Assignee
	Comment 11 • 17 years ago

no -- with this approach images will still claim to take up the same amount of space with the cache.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 17 years ago

Would be great to get this to M8 (in which lowering memory usage is high 
priority, IIRC).

	Stuart Parmenter Assignee
	Comment 13 • 17 years ago

not going to work on this anymore.  going to follow up with bug 296818 instead.