RE: Monitor.c patch

Hi Russell,

I will look at this soon.  I have actually started to do some of what you
have done so I will integrate it with my code.  There are things I see
you've done like change USE_HASHING_MONITORS to USE_SMALL_MONITORS.  I've
actually changed it to IL_CONFIG_USE_THIN_LOCKS (and added profiles to
support that)...etc etc.

I'll send a patch back your way when I'm done...

^Tum

> -----Original Message-----
> From: Russell Stuart [mailto:[email protected]]
> Sent: Wednesday, 5 May 2004 8:35 p.m.
> To: Thong (Tum) Nguyen
> Cc: [email protected]
> Subject: Monitor.c patch
> 
> I have put up a second patch for you to look at.
>  https://savannah.gnu.org/patch/index.php?func=detailitem&item_id=3009
> 
> This is the re-coding of how monitors are created and destroyed.  Given
> that you have fixed the race condition that was plaguing me (although
> I haven't checked it as yet), the original reason for doing this has
> gone.  Still, I think this patch is worth applying.  The reasons are:
> 
> 1.  In the current implementation, the number of global mutexes locked
>     by the typical "lock (...) ..." has gone from 3 to 0, hopefully
>     speeding it up.  I haven't attempted to measure whether it does
>     speed it up, though.
> 
> 2.  Quite a bit of common code has been factored out, hopefully making
>     it all a bit easier to understand.
> 
> 3.  I have removed the per thread free monitor queues.  They are a
>     potential memory leak.  They will leak if one queue creates the
>     monitors and another frees them.
> 
> 4.  The code that checks whether a monitor is "unused" and thus can be
>     freed has been simplified considerably.
> 
> Anyway, if I have got you interested, here are the changes I have made.
> Most all of the changes are to do with how monitors are created and
> destroyed.
> 
> a.  As I described earlier, non-hasing monitors are now only freed
>     on garbage collection.  Hashing monitors work as before.  I used
>     your suggestion of typed GC allocations to to make this work with
>     ILGCAllocAtomic.  The monitor is destroyed by a GC finaliser
>     attached to the monitor.
> 
> b.  When a monitor is created one global mutex is hit.  As before, the
>     monitor is installed with a CompareAndSwap, and this is where the
>     global monitor is used.  If someone re-writes CompareAndSwap to use
>     a hardware interlock, then there will be no global mutex used.
> 
> c.  As mentioned above, the per thread free list is gone.  In the
>     small monitor case there is a single global list.  This does not
>     slow down small monitors as then have to lock a global mutex
>     anyway when allocating or freeing a monitor.
> 
> d.  Although the fast monitor code only frees monitors via the garbage
>     collector, the code to free the monitor as soon as it is unused is
>     still present so it wouldn't be hard to change it back to the
>     old way.  The algorithm used to check if a monitor is unused has
>     changed, however.  Before it was rather complex, and included a
>     call to ILWaitMonitorCanClose.  The new algorithm takes advantage
>     of the fact that if you aren't in a Monitor.Enter()..Monitor.Exit()
>     block, then you can't use the monitor.  So a count of the number
>     of times such a block has been entered but not exited (including
>     waiting on Monitor.Enter()) is kept.  If that count is 0 the
>     monitor can be freed.
> 
> e.  The body of the _IL_Monitor_ routines used by the runtime (such
>     as _IL_Monitor_Exit) is unchanged, apart from catering to the new
>     monitor constructor / destructor code.  But they have all been
>     moved into pnet/engine/monitor.c.  Before the bulk (all?) of
>     them were in pnet/engine/lib_thread.c.  Only monitor.c knows
>     about VM monitors now.  This got rid of a few globals - for
>     example the new version of the ILExecMonitor structure is now
>     private to monitor.c
> 
> This has had the side effect of reducing the number of failures
> in TestMonitor.cs from 5 to 3.  I am not sure if that means anything -
> it may just be hiding a race condition.  If it has fixed a bug, it is
> probably because of the simplified "unused monitor" checking.
> 
> Some other metrics:
> 
>   Speed:          Unchanged, as least when measuring how fast the
>                   test suite runs.  This is probably a lousy test,
>                   but it does show its not dramatically different
>                   one way or the other.
> 
>   Lines of code:  About 180 lines shorter (not counting comment lines).
>                   If you want to check this yourself, I ran this
>                   shell line in the pnet directory to count lines (beware
>                   is contains tabs which have probably been munged by
>                   my mail client):
> 
> egrep -v '^[      ]*(//|/\*|\*/|\*[       ]|\*$|$)' \
>   $(find include engine support -name "*.[ch]") | wc -l
> 
> The small/hashing monitor case hasn't been tested yet - mostly because
> I wanted to get the non-hashing case testing cleaning before I did
> that.
>