[m-dev.] for review: fix :- export with threads memory leak

[Fergus Henderson]

On 27-Sep-2000, Tyson Dowd <trd at cs.mu.OZ.AU> wrote:
> Fix a bug with :- export and threads.  
> 
> Each time we called from C to Mercury, we initialized the thread engine
> (if necessary).  
> 
> This allocated a new context every time we entered Mercury.
> Unfortunately, these contexts were never released, so every entry into
> Mercury from C cost about 4Mb of memory (almost all of which is the
> deterministic stack).  In a busy CORBA application you would run out of
> memory really quickly.
> 
> compiler/export.m:
> 	When initializing threads, remember whether we are responsible
> 	for finalizing the engine (e.g. if we are the first C->Mercury
> 	call to create the engine, we'll be the last to exit it and should 
> 	clean up afterwards).
> 
> runtime/mercury_engine.c:
> 	Finalize engines by destroying the context (this will put the
> 	memory zones onto a free list).
> 
> runtime/mercury_thread.c:
> runtime/mercury_thread.h:
> 	Make init_thread return TRUE if an engine has been allocated and
> 	it is the caller's responsibility to finialize it.
> 
> 
> 
> 
> Index: runtime/mercury_engine.c
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury_engine.c,v
> retrieving revision 1.25
> diff -u -r1.25 mercury_engine.c
> --- runtime/mercury_engine.c	2000/09/14 11:12:16	1.25
> +++ runtime/mercury_engine.c	2000/09/27 01:46:01
> @@ -128,7 +128,7 @@
>  
>  void finalize_engine(MercuryEngine *eng)
>  {
> -	
> +	destroy_context(eng->this_context);
>  }

This should release all the other resources allocated by init_engine().
It would be a good idea to add comments here explaining for each one
why it isn't released, or XXX comments saying that they ought to be
released.

For the heap zones, it's a bit tricky; you can't safely deallocate the
heap zones, because there might still be live pointers into them.  I
suppose it would have to be the accurate GC's responsibility to
deallocate unreferenced heap zones.

The MR_LOWLEVEL_DEBUG dumpstack_zone stuff looks like it is completely broken
in the case of multiple threads or multiple engines.

> +++ runtime/mercury_thread.c	2000/09/27 01:47:40
> @@ -66,7 +66,7 @@
>  
>  #endif /* MR_THREAD_SAFE */
>  
> -void
> +MR_Bool
>  init_thread(MR_when_to_use when_to_use)
>  {
>  	MercuryEngine *eng;
> @@ -78,7 +78,7 @@
>  		** return, there's nothing for us to do.
>  		*/
>  	if (pthread_getspecific(MR_engine_base_key)) {
> -		return;
> +		return FALSE;
>  	}

Hmm, I think that should be calling MR_PTHREAD_GETSPECIFIC()
rather than pthread_getspecific().

> +/* 
> +** Release resources associated with this thread.
> +** XXX calling destroy_engine(eng) appears to segfault.
> +** This should probably be investigated and fixed.
> +*/
> +void
> +finalize_thread_engine(void)
> +{
> +#ifdef MR_THREAD_SAFE
> +	MercuryEngine *eng;
> +
> +	eng = pthread_getspecific(MR_engine_base_key);
> +	pthread_setspecific(MR_engine_base_key, NULL);
> +	finalize_engine(eng);
> +#endif
>  }

Likewise here you should call MR_PTHREAD_SETSPECIFIC
and MR_PTHREAD_GETSPECIFIC rather than pthread_getspecific
and pthread_setspecific.

The XXX comment here would be better placed immediately
before the call to finalize_engine().

Otherwise, that change looks fine.  But it would be good if you
could investigate that XXX comment.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>  |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3        |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------