413050 - call cairo_debug_reset_static_data in system cairo on shutdown

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect, P2)

Description

[Karl Tomlinson (:karlt)]

Currently we call cairo_debug_reset_static_data for tree cairo in memory analysis and debug builds.

The system cairo has large font caches as used by Pango and GTK.

We should use cairo_debug_reset_static_data on the system cairo in these builds also, to empty these caches.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: patch

This calls the system cairo_debug_reset_static_data after closing the display.
It also disables calling this function (an extra time) in gfxPlatform::~gfxPlatform when building with system cairo, at which time gtk may still be using the data.

The gtk+-2.8.0 version check is because this is when gtk started using cairo (in stable releases).  We only support 2.10 for our builds.

The cairo.h header is pulled in from gtk.h.  I don't want to include it explicitly because system-headers only wraps cairo.h when using system cairo only.

Comment 2

[Karl Tomlinson (:karlt)]

Old data (bug 362682 comment 34) show that the reduction in shutdown leaks should be significant (measured reduction from 7151329 to 2424366, but exactly how much will depend on the fonts installed).

Comment 3

[Karl Tomlinson (:karlt)]

Blocking request based on bug 414388 comment 5.

Comment 4

[Benjamin Smedberg]

Comment on attachment 297914 [details] [diff] [review]
patch

I don't understand... it looks like in the case on in-tree cairo we would be calling cairo_debug_reset_static_data twice... is this the desired behavior? Why not just call it once in nsAppRunner, if that's what's necessary?

And/or, why couldn't we make the call in ~gfxPlatform in both cases?

I think vlad/stuart should review this, not me.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 297914 [details] [diff] [review]
patch

(In reply to comment #4)
> I don't understand... it looks like in the case on in-tree cairo we would be
> calling cairo_debug_reset_static_data twice... is this the desired behavior?
> Why not just call it once in nsAppRunner, if that's what's necessary?

It is intentional to call that function for both the tree cairo and the system cairo (if compiled with in-tree cairo).

> And/or, why couldn't we make the call in ~gfxPlatform in both cases?

gfxPlatform includes cairo.h from mozilla/gfx/cairo/cairo/src, which defines 
cairo_debug_reset_static_data as _moz_cairo_debug_reset_static_data, and so that function gets called.  ~gfxPlatform is the right place to call that function because that is when we have finished with the tree cairo.

nsAppRunner includes cairo.h from /usr/include/cairo and it is the system cairo that is used by GTK and Pango.  Calling cairo_debug_reset_static_data for that cairo library should be done after closing the display.

> I think vlad/stuart should review this, not me.

Passing on to stuart.

Comment 6

[Stuart Parmenter]

Comment on attachment 297914 [details] [diff] [review]
patch

go for it

Comment 7

[Karl Tomlinson (:karlt)]

Lk 3.79 -> 1.15

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1202798792&maxdate=1202798851&who=karlt%2B%25karlt.net

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is it really safe to assume that we'll always use an in-tree cairo on non-X11 platforms?

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to comment #8)
I'm guessing you are referring to the fact that _cairo_debug_reset_static_data is now not called at all for non-MOZ_WIDGET_GTK2 non-MOZ_TREE_CAIRO builds.

It's definitely "safe" not to call that function.  I'm claiming that it was _not_ safe to call that function on a system cairo from ~gfxPlatform, as gfxPlatform does not know what other use the application might have for the library.

The disadvantage now is that people who run memory tests on non-MOZ_WIDGET_GTK2 non-MOZ_TREE_CAIRO builds are now going to have (many) more shutdown leaks.  The system-headers issue makes it non-trivial to include cairo.h in nsAppRunner for these other platforms.  Would be nice to sort it out, but I doubt there's many people to notice.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #5)
> gfxPlatform includes cairo.h from mozilla/gfx/cairo/cairo/src, which defines 
> cairo_debug_reset_static_data as _moz_cairo_debug_reset_static_data, and so
> that function gets called.  ~gfxPlatform is the right place to call that
> function because that is when we have finished with the tree cairo.
> 
> nsAppRunner includes cairo.h from /usr/include/cairo and it is the system cairo
> that is used by GTK and Pango.  Calling cairo_debug_reset_static_data for that
> cairo library should be done after closing the display.

What guarantees that those cairo.h's are included by those files?  (Is it the "< vs <> include path differences?)  Can we assert that with #error somehow?  e.g.:

#ifdef cairo_debug_reset_static_data
#error "We're including Mozilla's cairo instead of system cairo"
#endif

?

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to comment #9)
> The system-headers issue makes it non-trivial to include cairo.h in nsAppRunner
> for these other platforms.

Actually it probably is quite trivial, because are no system-headers issues when not MOZ_TREE_CAIRO.  We'd just need some modifications to Makefiles to set include paths.  So I think this can be done if anyone is going to notice.

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to comment #10)
> What guarantees that those cairo.h's are included by those files?  (Is it the
> "< vs <> include path differences?)  Can we assert that with #error somehow? 
> e.g.:

It's just the differences in include paths (set in Makefiles).

> #ifdef cairo_debug_reset_static_data
> #error "We're including Mozilla's cairo instead of system cairo"
> #endif

Sounds a good idea.