[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[DotGNU]Monitor.c patch

From: Russell Stuart
Subject: [DotGNU]Monitor.c patch
Date: 05 May 2004 18:35:12 +1000

I have put up a second patch for you to look at.

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

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]