[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.
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.
- [DotGNU]Monitor.c patch,
Russell Stuart <=