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

[Tyson Dowd]

On 27-Sep-2000, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> 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.

Yes, that would be nice, but I don't really want to spend all the time
require to find out exactly what resources are not released.  But I can
definitely comment that someone should do this.

> 
> 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.

Certainly it's something that needs to be tested with a working agc
grade.

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

Zoltans comment seems pertinent -- although I don't want to make that
change along with this change because I want to fix this bug and not
introduce others.

> 
> > +++ 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().

Ok, that's cool.

> 
> > +/* 
> > +** 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().

Ok.

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

Sure would.  If it were causing any memory to leak I would, but it
appears that only the det stack was causing memory to be allocated.
I will put it on the list of things for a rainy day.

-- 
       Tyson Dowd           # 
                            #  Surreal humour isn't everyone's cup of fur.
     trd at cs.mu.oz.au        # 
http://www.cs.mu.oz.au/~trd #
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------