395301 - Move offscreen surface creation from gfxOS2Platform to gfxOS2Surface

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Peter Weilbacher]

Currently, the offscreen surface is created in gfxOS2Platform and its presentation handle mPS is kept around until the gfxOS2Platform is destroyed, i.e. until program shutdown. We should really do it like the other platforms and use "new gfxOS2Surface" with suitable arguments in gfxOS2Platform and then keep the handle around in the surface class instead.

I already have a patch to do that but I need to clean it up first...

Comment 1

[Andy Willis (abwillis)]

I wonder if that is why I am seeing so much memory usage in a very short time, if not for the highmem build I wouldn't be able to run long at all.
Do we need to explicitly create an offscreen surface?  I thought our Cairo implementation did its own offscreen work in order for us to have the alpha transparency.

Comment 2

[Peter Weilbacher]

Yes, we need that. Without it we couldn't build. And as far as I understand things painting wouldn't happen without it, at least the code that creates certain elements (images, SVGs) depends on the offscreen surface.
And yes, cairo does double buffering, too. But I think that is the same for every platform.

Comment 3

[Peter Weilbacher]

Attachment: Do it

OK, this should do it. This patch applies on top of the latest one from bug 371505.

The old way could indeed have introduced severe memory leaks. Because every time, CreateOffscreenSurface was called, we created a new memory context etc., but we could only remove the related memory (the DC itself and the contained PS and Bitmap) in the destructor (on program shutdown). And by then the same function was called many more times, each time resetting the variables mDC, mPS, and mBitmap to new values. The effect would have been that only at on shutdown and only the very latest offscreen surface was destroyed!

With this patch the offscreen surfaces also get destroyed in the gfxOS2Surface destructor (like the surfaces that are connected to a window) and that is apparently called at least when a window is closed. (But that means that if somebody doesn't open new windows from time to time and closes the old ones, the app will grow as before.)

Comment 4

[Andy Willis (abwillis)]

I would guess that:
+#define DEBUG_thebes_2
+

is in gfxos2surface.cpp for testing?  
I'll try the build with it now.

Comment 5

[Andy Willis (abwillis)]

It built and loads fine... in fact pretty well even under Battery Optimized. I can't tell it did anything for the memory usage... closing a chatzilla tab uses 4M (uses not releases). Just checked and opening a new tab in the Browser uses 3 meg and then 4 meg to close it.

Comment 6

[Peter Weilbacher]

Oops, yes, forgot the debugging #define. Will remove that when checking in. Before that we need to look a bit more into what causes the memory increase when closing windows (which I do see) and tabs (which I cannot confirm) and if that has changed with this patch (even though I don't see how).

Comment 7

[Peter Weilbacher]

It just occurred to me that closing tabs could create extra memory usage because of the "Undo Close Tab" functionality. But I haven't cross tested this with a version without this patch of against branch yet.

Comment 8

[Andy Willis (abwillis)]

Just resizing a Mozilla window increases memory use.  Not sure if that is related to this bug or is it is in bug 371505.

Comment 9

[Peter Weilbacher]

That is indeed shocking. I would expect quite a bit of increase when resizing the window because every pixel of the window area is represented by 4 byte in RAM (cairo's ARGB32 format). But it really becomes a problem as the memory consumption does not go down when making the window smaller again. Just by resizing the same window with the same webpage to different sizes I just made it consume 100 MB more. And this was just with one tab open...

Comment 10

[Peter Weilbacher]

Attachment: move offscreen v2

OK, given that bug 371505 badly affects RAM usage (leaks?!), I think I should get this change in without depending on that. It won't fix the flicker, but it actually improves RAM usage, too (up to ~10% less RAM used in my tests). This patch would do that.

[When getting something along the lines of bug 371505 in, the 
   gfxOS2Surface(HPS aPS, const gfxIntSize& aSize)
 constructor and the mOwnsPS variable could then go.]

Comment 11

[Peter Weilbacher]

Fix checked into trunk.