[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[DotGNU]RE: Monitor.c patch
From: |
Thong (Tum) Nguyen |
Subject: |
[DotGNU]RE: Monitor.c patch |
Date: |
Wed, 5 May 2004 20:46:01 +1200 |
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:address@hidden
> Sent: Wednesday, 5 May 2004 8:35 p.m.
> To: Thong (Tum) Nguyen
> Cc: address@hidden
> 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.
>