[m-rev.] For review: Implement support for threadscope profiling.

[Paul Bone]

On Wed, Dec 02, 2009 at 12:19:23PM +1100, Peter Wang wrote:
> On 2009-12-01, Paul Bone <pbone at csse.unimelb.edu.au> wrote:
> > 
> > For review by anyone.
> > 
> > Support for threadscope profiling of the parallel runtime.
> > 
> > This change adds support for threadscope profiling of the parallel runtime in
> > low level C grades.  It can be enabled by compiling _all_ code with the
> > MR_PROFILE_PARALLEL_EXECUTION_SUPPORT C macro defined.  The runtime, libraries
> > and applications must all have this flag defined as it alters the MercuryEngine
> > and MR_Context structures.
> 
> This might require a new grade component, later.

I was thinking about this.

> > runtime/mercury_context.c:
> > runtime/mercury_context.h:
> >     Add a new function for pinning the primordial thread.  If the OS supports
> >     sched_getcpu we use it to determine which CPU the primordial thread should
> >     use.  No other thread will be pinned to this CPU.
> >     Add a numeric id field to each context, this id is uniquely assigned and
> >     identifies each context for threadscope.
> 
> >     Move MR_load_context from a C macro to a C procedure.
> 
> Are you sure about this?
> 
> Unless you have tested on SPARC, I suggest you revert this part for now.
> I just have a feeling it might be a problem with register windows.
> Someone else might know for sure.

I've probably broken compatibility with non x86 previously in other changes
anyway.  But there's no reason to break it further, I agree.  (One day I'd like
access to a Sparc T2, so I'll be interested in this).

I'll revert this.

> (Please use blank lines between changes.)

Okay, I've never been sure what was recommended.

> > Index: runtime/mercury_context.c
> > ===================================================================
> > RCS file: /home/mercury1/repository/mercury/runtime/mercury_context.c,v
> > retrieving revision 1.71
> > diff -u -p -b -r1.71 mercury_context.c
> > --- runtime/mercury_context.c	27 Nov 2009 03:51:19 -0000	1.71
> > +++ runtime/mercury_context.c	1 Dec 2009 05:31:19 -0000
> > @@ -96,14 +94,28 @@ static MR_Integer       MR_profile_paral
> >  static MR_Integer       MR_profile_parallel_regular_context_reused = 0;
> >  static MR_Integer       MR_profile_parallel_small_context_kept = 0;
> >  static MR_Integer       MR_profile_parallel_regular_context_kept = 0;
> > +#endif
> >  
> >  /*
> > -** Write out the profiling data that we collect during execution.
> > +** Local variables for thread pinning.
> >  */
> > -static void
> > -MR_write_out_profiling_parallel_execution(void);
> > +#if defined(MR_THREAD_SAFE) && defined(MR_LL_PARALLEL_CONJ) && \
> > +    defined(MR_HAVE_SCHED_SETAFFINITY)
> 
> MR_LL_PARALLEL_CONJ implies MR_THREAD_SAFE so you can shorten these
> conditionals.

*Phew*

> > +static MercuryLock      MR_next_cpu_lock;
> > +MR_bool                 MR_thread_pinning = MR_FALSE;
> > +static MR_Unsigned      MR_next_cpu = 0;
> > +#ifdef  MR_HAVE_SCHED_GETCPU
> > +static MR_Integer       MR_primordial_threads_cpu = -1;
> > +#endif
> > +#endif
> 
> MR_primordial_thread_cpu (i.e. not plural)

it was supposed to be possessive (with an apostrophe) but I think it's better
without the 's'.

> > +static MercuryLock      MR_next_context_id_lock;
> > +static MR_ContextId     MR_next_context_id;
> >  #endif
> 
> I didn't see these two locks being initialised, but I might have missed it.

You didn't, I had forgotten.

> Just a note: I prefer not to have so many trivial locks.  If contention is
> unlikely to be high, I think sharing or reusing locks is better.
> e.g. MR_next_context_id_lock looks redundant.  MR_create_context acquires
> free_context_list_lock; you could increment MR_next_context_id then.

Good idea, I've done this.

> > Index: runtime/mercury_engine.h
> > ===================================================================
> > RCS file: /home/mercury1/repository/mercury/runtime/mercury_engine.h,v
> > retrieving revision 1.50
> > diff -u -p -b -r1.50 mercury_engine.h
> > --- runtime/mercury_engine.h	30 Oct 2009 03:33:28 -0000	1.50
> > +++ runtime/mercury_engine.h	30 Nov 2009 10:34:15 -0000
> > @@ -392,6 +392,16 @@ typedef struct MR_mercury_engine_struct 
> >  #ifdef  MR_THREAD_SAFE
> >      MercuryThread       MR_eng_owner_thread;
> >      MR_Unsigned         MR_eng_c_depth;
> > +#if defined(MR_LL_PARALLEL_CONJ) && defined(MR_PROFILE_PARALLEL_EXECUTION_SUPPORT)
> > +    /*
> > +    ** For each profiling event add this offset to the time so that events on
> > +    ** different engines that occur at the same time have the same time in
> > +    ** clock ticks.
> > +    */
> > +    MR_int_least64_t                    MR_eng_cpu_clock_ticks_offset;
> > +    struct MR_threadscope_event_buffer  *MR_eng_ts_buffer;
> > +    MR_Unsigned                         MR_eng_id;
> > +#endif
> >  #endif
> 
> MR_eng_id and MR_ctxt_num_id would probably be useful for other reasons,
> so you could move them out of the conditionals in a later change.

I agree.  This will mean that the MR_Context structure doesn't change when
MR_PROFILE_PARALLEL_RUNTIME_SUPPORT is defined.

> I skipped over mercury_threadscope.[ch]

That's okay.  They'll probably have a number of modifications in the near
future.  And, in the short term I'll probably be the single user of this
feature.

Thanks for your review, I've fixed up everything else and will commit this
after double checking some things (I want to re-run the bootcheck).

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 489 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20091202/53c3e31c/attachment.sig>