Bug 34842

Created attachment 43936 [details]
Leak 2

Created attachment 43937 [details]
Leak 3

Thanks for the bug reports.

The first two leaks are not real leaks; they are simply one-time allocations that are needed for the lifetime of the applications.

The third one is a real leak, and one we know about already. Unfortunately, I don't know enough about Windows to fix it myself, and there hasn't been sufficient interest in pixman on Windows for anyone else to fix it. The leak only happens once per thread, so unless you are spawning a lot of threads, it probably isn't a big deal. If your program is single-threaded or only calls pixman from one thread, then you can define PIXMAN_NO_TLS to work around the issue. 

What's going on is that PIXMAN_DEFINE_THREAD_LOCAL on mingw32 expands to something that calls calloc() whenever the thread local storage is initialized. What we need is some way to call free() when the thread exists, but I am not sure how to do that.

(In reply to comment #3)
> Thanks for the bug reports.
> The first two leaks are not real leaks; they are simply one-time allocations
> that are needed for the lifetime of the applications.


Does that mean you rely on the operating system to free that memory?  I suppose this is OK, but I didn't think it was good practice in general.  Could you provide a function to free this memory?

> The third one is a real leak, and one we know about already. Unfortunately, I
> don't know enough about Windows to fix it myself, and there hasn't been
> sufficient interest in pixman on Windows for anyone else to fix it. The leak
> only happens once per thread, so unless you are spawning a lot of threads, it
> probably isn't a big deal. If your program is single-threaded or only calls
> pixman from one thread, then you can define PIXMAN_NO_TLS to work around the
> issue.

I don't see PIXMAN_NO_TLS anywhere.
 
> What's going on is that PIXMAN_DEFINE_THREAD_LOCAL on mingw32 expands to
> something that calls calloc() whenever the thread local storage is initialized.
> What we need is some way to call free() when the thread exists, but I am not
> sure how to do that.

I don't believe Windows provides a way to catch threads on exit, but couldn't you just provide a function like pixman_clean_cache() or something? It would be the responsibility of the calling application to call that from each thread before it closes (if it cares about the leak).  Alternatively I guess you could attempt some sort of reference counting to the cache, but that would seriously reduce the value of the cache.




I understand that these leaks are not significant in my application because I only call pixman from one thread.  However, I do like to use _CrtSetDbgFlag(_CRTDBG_LEAK_CHECK_DF | _CRTDBG_ALLOC_MEM_DF); to check for my own memory leaks.  It would be nice if there were function I could call (perhaps in DEBUG builds only) that would clear the cache and this other memory so that it doesn't show up in the leak check when my application exits.

I am fairly familiar with Windows programing, so if you have any thoughts, I would be glad to help.

(In reply to comment #4)
> (In reply to comment #3)
> > Thanks for the bug reports.
> > The first two leaks are not real leaks; they are simply one-time allocations
> > that are needed for the lifetime of the applications.
> 
> 
> Does that mean you rely on the operating system to free that memory?  I suppose
> this is OK, but I didn't think it was good practice in general.  Could you
> provide a function to free this memory?

It's generally better to rely on the operating system to free that memory. I wouldn't necessarily be completely opposed to a patch designed to make cairo_debug_reset_static_data() work better, but in general, there is no point freeing memory just before exiting an application.

> > The third one is a real leak, and one we know about already. Unfortunately, I
> > don't know enough about Windows to fix it myself, and there hasn't been
> > sufficient interest in pixman on Windows for anyone else to fix it. The leak
> > only happens once per thread, so unless you are spawning a lot of threads, it
> > probably isn't a big deal. If your program is single-threaded or only calls
> > pixman from one thread, then you can define PIXMAN_NO_TLS to work around the
> > issue.
> 
> I don't see PIXMAN_NO_TLS anywhere.

It is in pixman-compiler.h

> > What's going on is that PIXMAN_DEFINE_THREAD_LOCAL on mingw32 expands to
> > something that calls calloc() whenever the thread local storage is initialized.
> > What we need is some way to call free() when the thread exists, but I am not
> > sure how to do that.
> 
> I don't believe Windows provides a way to catch threads on exit, but couldn't
> you just provide a function like pixman_clean_cache() or something? It would be
> the responsibility of the calling application to call that from each thread
> before it closes (if it cares about the leak).  Alternatively I guess you could
> attempt some sort of reference counting to the cache, but that would seriously
> reduce the value of the cache.

In general applications don't have the necessary information to do this reliably. Consider the case where some underlying library has spawned a thread that is using pixman for compositing. The application has no way to know hwen that thread exited. 

I believe there is a way to catch thread exits in windows by using the DllMain() function.

> I am fairly familiar with Windows programing, so if you have any thoughts, I
> would be glad to help.

It would definitely be useful to make sure the thread specific memory gets freed using the DllMain() function.

An alternative might be to make the cache in question not thread local, but instead use a critical region to protect accesses. The cache in question is somewhat performance critical, so some sort of lock striping might be necessary to prevent (CPU-) cache thrashing.

> It's generally better to rely on the operating system to free that memory. I
> wouldn't necessarily be completely opposed to a patch designed to make
> cairo_debug_reset_static_data() work better, but in general, there is no point
> freeing memory just before exiting an application.

It would be nice to avoid leak detection tools from reporting.  A function that would only be called in DEBUG builds to clean that memory would satisfy that need.

> In general applications don't have the necessary information to do this
> reliably. Consider the case where some underlying library has spawned a thread
> that is using pixman for compositing. The application has no way to know hwen
> that thread exited. 
> I believe there is a way to catch thread exits in windows by using the
> DllMain() function.

I suppose I mispoke.  In the circumstance you describe the library that spawned the thread could be responsible for cleaning the cache.  I think that in general it's not possible for pixman to be able to determine the appropriate time to clean its cache.

> It would definitely be useful to make sure the thread specific memory gets
> freed using the DllMain() function.
> An alternative might be to make the cache in question not thread local, but
> instead use a critical region to protect accesses. The cache in question is
> somewhat performance critical, so some sort of lock striping might be necessary
> to prevent (CPU-) cache thrashing.


You are correct.  The DllMain() function does get called for each loaded library whenever a process or thread is created or terminated.  This is an appropriate locatin for TLS cleanup for a DLL.  I didn't mention that before because my particular situation uses pixman as a static library.  The DllMain() function does not apply to static libraries.  It seems like for a static library a function to clean thread specific data would be required since pixman is never "loaded".  I wouldn't think providing such a function would be bad, because it would be essentially the same function that would be called from DllMain().

Windows users are encouraged to do a research on this issue and propose a patch. I doubt that anyone else is eager to setup the necessary development environment to work on resolving it.

I make a patch based on the cairo-1.14.4 and pixman-0.32.8

Created attachment 120013 [details] [review]
add two additional functions in cairo_debug_reset_static_data

Created attachment 120014 [details]
Add functions: _cairo_win32_device_reset_static_data

Created attachment 120015 [details] [review]
patch_for_34842

Add two additional functions to reset win32-device static data and pixman implementation static data, when the application quit, please call cairo_debug_reset_static_data, then memory leak won't happen.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/pixman/pixman/issues/13.