RE: Monitor weirdness

I'll reply to this fully but just gotta remove Russell Fitchett from the CC.
That was a mistake.  That example code was mine not Russell's.  I fudged the
CC list because your names are so similar :-).

> -----Original Message-----
> From: Russell Stuart [mailto:russell-M7Clnf/[email protected]]
> Sent: Thursday, 6 May 2004 6:33 p.m.
> To: Thong (Tum) Nguyen
> Cc: [email protected]; 'Russell Fitchett'
> Subject: RE: [DotGNU]Monitor weirdness
> 
> On Thu, 2004-05-06 at 12:59, Thong (Tum) Nguyen wrote:
> > I'm not convinced your MonitorAbortDuringWait MonitorInterrup* tests are
> > accurate.  Your test tests to see if a waiting thread that is aborted
> > reaquires the lock that it was waiting on.  By my tests, MS.NET does not
> > require that a thread reaquire a lock.  It does the "pretend" reaquire
> thing
> > where Wait is allowed to exit even though the monitor hasn't been
> aquired.
> > It also lets the thread call an appropriate number of "Exits" even
> though
> > the thread doesn't really own the lock.
> 
> Hmmm.  I did try to check for the very thing you are describing in the
> tests.  I had another look - and in two cases I got it wrong.  I tried
> to eliminate the "pretend" bit by:
>    lock (..) {
>      :
>      thread.Abort()/thread.Interrupt();
>      Thread.Sleep(2*1000);
>      this.seen = true;
>    }
> and verifying that "seen" was in fact true when Monitor.Wait(...)
> finally exited.  If it was true, there is no "pretending" going on - the
> lock was definitely re-acquired.  In the Monitor*DuringWait cases I got
> it right.  Ergo they are not pretending under MS.
> 
> I got it wrong for the Monitor*AfterWait tests.  When I corrected the
> test, I got the result you describe (ie the two Mointor*AfterWait tests
> fail under MS fail) - there obviously is some pretending going on.
> 
> I have now put the corrected code back into CVS.
> 
> Just so it is clear, I will spell out the difference between the two
> tests.  As you are aware, Monitor.Wait does two waits:
>   wait for pulse.
>   wait to re-aquire monitor lock.
> The Monitor*DuringWait tests aborting/interrupting the first wait.
> These all pass under MS.  The Monitor*AfterWait tests
> aborting/interrupting the second wait.  These fail under MS.
> 
> I have posted Russell's example code to
> nttp://microsoft.public.dotnet.general.  So far I have one response.  I
> wouldn't place too much faith in it, but the responder said he thought
> it was a bug.
> 
> Finally, the way the TestMonitorAbortEnter test fails, at least for me,
> is decidedly odd.  (It doesn't fail under MS.)  I get this result:
>   Test failed: Monitor.Enter threw an exception we couldn't catch!
> Icky, to say the least.  Something has gone badly wrong.
> 
> > Is this correct behaviour?  I don't think so.  Java doesn't allow a
> thread
> > to be interrupted if it can't reaquire the lock it needs to exit wait
> with a
> > consistant state.  Java's behaviour with stop (java's equivalent of
> abort)
> > seems to be that it allows the thread to exit wait without aquiring the
> lock
> > as well.  I would like to point out now that Sun wasn't being stupid
> when
> > the depreciated Thread.stop.
> 
> Yes, I agree.
> 
> > BTW, I'm still looking over your code.  I'm integrating some of your
> ideas
> > with the existing code.  I still believe that the current algorithm is
> > faster and by my tests, it can be 4-10 times faster (though in typical
> > desktop apps it may not make too much difference).  The current
> algorithm is
> > very fast when the monitor is uncontested and doesn't require all those
> > "global locks" you mentioned except for one compare and exchange.
> 
> Hmmm.  My first reaction was impossible - I know my critical path is
> shorter than yours!  But I rang my own tests and got the same result.
> Errk!
> 
> But I hadn't noticed that you had added ILWaitMonitorFastClaim while I
> was writing my version of monitor.c.  I altered my code to use it, and
> now it is faster than yours.  It ranges from being about the same for
> the wait test, to twice as fast when there is contention.  The lock
> (...) without contention (which I assume is the typical case), runs
> about 1.5 times as fast.  The benchmark I used to get these results is
> attached.  The new version of the patch has been uploaded to savannah.
> 
> Unless I am missing something, I don't see how my new version could
> possibly ever be slower than your code given they use the same routines
> in ILWait*.  The only reason it was slower before is that they weren't
> yours used the new ILWaitMonitorFastClaim.
> 
> BTW, there is no reason for ILWaitMonitorFastClaim to check for
> Interrupts or Aborts.  Interrupts do not take effect until the thread
> next blocks, ie is in the WaitJoinSleep state.  The MS Doco actually
> says this explicitly.  Also I have written code to verify it is the case
> under MS.  God knows what aborts do - but since they can take effect any
> time, there hardly seems any point in testing for them in the critical
> path.
> 
> >   On most
> > platforms where the CAX is optimised, Monitor.Enter will require no
> context
> > switches to the kernel because it never has to call
> ILWaitMonitorTryEnter
> > (which uses kernel level locks).  There are a lot of good ideas like not
> > incrementing monitor->waiters with InterlockedIncrement if
> > InterlockedIncrement is implemented using global locks.  I've decided
> that
> > putting monitors on the GC heap is a good idea.  If a lock is entered
> > without being exited before it gets GC-ed then the monitor will leak
> unless
> > it is GC allocated.  The currently implementation of ILWaitMutexClose
> > doesn't allow mutexes to close while still being owned so your
> > implementation (without the fixes) leaks as well.
> 
> Well - both implementations would leak for the same reason.  I did
> assume ILWaitHandleClose would be the equivalent of a Destroy.  If it
> isn't then perhaps we should add a Destroy.
> 
> > I also still like thread
> > local free lists for monitors -- combined with the current algorithm it
> > gives good performance.
> 
> But till not as fast as my implementation ... yet.
> 
> > Leaks can be prevented by limiting the free list to
> > a reasonable number of monitors (32?).  Keeping count the number of
> monitors
> > (etc) on the list will be fast and lock free because the lists are
> > thread-local.
> 
> This is additional complexity.  Right now, you have an example of
> something that is about 200 lines shorter, and runs up to twice as
> fast.  If you can beat that then start thinking about adding complexity.
> 
> > FYI, the reason the current monitor implementation failed 5 tests
> instead of
> > 3 is because Monitor.Wait assumed that the monitor is only reclaimed the
> > call to ILWaitMonitorWait succeeds without being aborted or interrupted.
> If
> > there is an abort or interrupt request it assumes that the monitor
> hasn't
> > been reaquired.  As mentioned above, the correct behaviour is ambiguous
> but
> > I'm leaning towards the Sun's implementation rather than Microsoft's
> (it's
> > also easier to implement ;-)).
> 
> OK.
> 
> > Anyway, try the following program (put a break point in the
> > ThreadInterruptedException handler) and cry like I did.  I'd be pretty
> keen
> > to meet with you on IRC and chat about threading issues further...
> 
> Hmmm.  I have just read this ... I possibly missed you.  You can always
> get me on MSN/ICQ/Yahoo at any time.  One day I will all Jabber as
> well.  My IM handles are at the end of this page:
> 
> http://www.lubemobile.com.au/ras/agenda/
> 
> Finally, back to where this all started.  My real program still hangs
> ... and it doesn't use interrupt or abort.  It hangs in both your and my
> version of monitor.c.  I added some trace to monitor.c, matched up every
> entrance to Monitor.Enter to its return.  Ie .. the program is not
> waiting in Monitor.Enter.  It doesn't (or at least my code doesn't) use
> Monitor.Wait.  I don't have the faintest idea about what is going on.  I
> am off to investigate.