Expressions in C#...

B

Bill Butler

Atmapuri said:
Hi!


If you would have read a few posts down, you would have found it.

I saw it right after I posted...It wasn't complete but it was enought to
find your problem.
The problem in your code is ~Vector1() [~Outer() in the sample below]
You assume that since the Outer object is being GCed it is safe to
assume that nobody holds a reference to the inner object.
This is a bad assumption.
In the sample program below this is demonstrated by the call
Func(outer.Inner);
We pass a reference to the inner object to Func.
GC is kicked off while inside of func (by the call to Outer outer = new
Outer(); )
outer is fair game for GC but Inner is still alive.

I don't know if your real example requires ~Vector1().
If not...let the framework handle it

Good Luck
Bill


Here is a short but complete program that demonstrates your problem
-------------------------
using System;
public class Inner
{
public int Value = 100;
}

public class Outer
{
public Inner Inner;

public Outer()
{
Inner = new Inner();
}

~Outer() {Inner.Value = 0;}
}

public class Test
{
public static void Main()
{
for (int i=0;i<=10000000;i++)
{
Outer outer = new Outer();
Func(outer.Inner); // since outer is not referenced again it is
fair game for GC
// outer.Inner.Value++; // This statement can fix the problem by
referencing outer
}
}

public static void Func(Inner Inner)
{
Outer outer = new Outer(); // this line triggers the GC to occur
eventually
if (Inner.Value == 0){ throw new Exception("Ooops"); }
}
}
 
A

Atmapuri

Hi!
I saw it right after I posted...It wasn't complete but it was enought to
find your problem.
We pass a reference to the inner object to Func.
GC is kicked off while inside of func (by the call to Outer outer = new
Outer(); )
outer is fair game for GC but Inner is still alive.

That is not the reason. You can comment out GC.Collect and
the error still happens, but it is much harder to make it trigger.
You have to run it for extensive times and it will happen...

The GC anyway keeps the reference count that references
the same object. According to the post of one of MVP's
the object reference is kept on the stack and that reference
will not be collected until the procedure ends.

So, the problem here is that the object itself is not collected
but its finalizer is called.... It looks to me at least as a finalizer
bug.... What I need to know for certain is if this problem goes
100% away if Vector1 is a struct and the finalizer is moved
to the internal object.

As for your example, as soon as you have assigned the
object to local variable before passing it to the function, you
worked around the problem. For example.

temp = 2*c;
a = Func(temp);

Works fine.... But

a = Func(2*c)

fails.... But that seriously affects the use of expressions...
(Should not).
I don't know if your real example requires ~Vector1().
If not...let the framework handle it

Yes it absolutely does. A part from that, have you heard of
any major library that makes extensive use of overloaded
operators? There are two types of problems:

1.) Heavy load on the garbage collector and discuraged by
Microsoft. This was one of the first articles on .NET and
how not to write performance code...
The bigger the object, the bigger the problems..
2.) Expression bugs...?!! Maybe...:)

Thanks!
Atmapuri
 
C

Christof Nordiek

Hi,

why you are cleaning up the Data inside the Vector1 class.
In the implicit cast operator you return a reference to the very!!! same!!!
instance
of that class. Then in Dispose(bool) (called by the finalizer) you set the
length to zero. This may change an instance referenced by another variable.

In the two lines, where the Exception is thrown you check for vector.Length
== 0.
I suppose x.Length was set to 0 in the finalizer of a Vector1-Instance.

1. You should really think, if you have to change Data.Length in the
Dispose-Method.
2. You really shouldn't cleanup Data inside of the Finalizer.
Since Vec1 is a managed class it will be cleaned up automatically at that
point if it is
not referenced by another field.
3. A cast operator really shouldn't return an inner instance of a mutable
class.

HTH
 
M

Marc Gravell

Sorry, but I have to agree with Bill; you are simply using finalizers wrong
here.
Short version; you don't need finalizers here. They are not helping you *at
all*, and are biting you hard on the hand. Lose them.

Longer version:
In particular, a finalizer should *not* be talking to CLR references that it
holds; in your case you /are/ being bitten by the inner object still bing in
use, but you are actually lucky you didn't get hit by the other gotcha:
accessing a heap item that has already been finalized and canned.

Finalizers are a last-chance for cleaning up managed resources. Generally
this should be done via IDisposable, but finalizers are offered to attmpt to
give the module developer a chance to at least cope with bad programmers
downstream. Elligible GC objects are destroyed in an undefined sequence -
they have to be, otherwise it could never clean up loops of
mutually-referenced objects - hence if your object is abandoned there is
every chance that "Data" has already been expunged.

Dispose and finalizers are *not* intended as a place to deconstruct your
objects (clearing any collections, etc) - leave the GC alone, it will deal
with this for you; in fact, attempting to do this can actually *increase*
memory usage, as it could well be that some class's Clear() method actually
allocates a new internal collection - so you may well be creating objects
unnecessarily.

The only real-life case I have found of legitimately using a finalizer on a
pure managed object goes as follows; its sole purpose is to chastise
developers who fail to call Dispose! (I think there may have been some #if
DEBUG PP-directives etc, but...):

public abstract class GrumpyDisosable : IDisposable {
public void Dispose() {
GC.SuppressFinalize(this);
Dispose(true);
}
public GrumpyDisosable() {}
~GrumpyDisosable() {
// coded here to prevent hiding by override; note can't use
ToString() as
// may reference destroyed objects
Debug.WriteLine("You abandoned me! Can we say \"commitment
issues\"?", GetType().FullName);
Dispose(false);
}
protected virtual void Dispose(bool disposing) {}
}
 
M

Marc Gravell

sorry - I dropped an "un" and broke the entire meaning!

Finalizers are a last-chance for cleaning up **UN**managed resources

Typically this means old-style handles to files, UI, sockets, etc.
 
A

Atmapuri

Hi!

All the complaints about the way the code
is written have to do with missing code. What you dont
understand is not neccessary without meaning...

The CLR is managed and should not be failing if all
its objects are managed no matter what....

Thanks!
Atmapuri
 
M

Marc Gravell

The CLR is managed and should not be failing if all
its objects are managed no matter what.... ....
They are the "only" way to clean them up within expressions.

If the objects are managed, you shouldn't be using finalizers. With perhaps
the one example I already mentioned.

Marc
 
M

Marc Gravell

For ref, I have varified that your code does crash as you describe.

Removing the unnecessary finalizer fixes it, as does moving the finalizer to
the Vec1 class, so that Vec1 cleans *itself* up when the GC decides it is
genuinely out of scope.

If there /is/ a genuine need for finalizer here, then it isn't apparent in
your code, and possibly it isn't quite following a robust pattern. But I
simply can't fault the CLR here - it is doing its job very well.

Marc
 
W

Willy Denoyette [MVP]

Well, you introduced a bug in your code , let me try to explain how you did
...

| protected virtual void Dispose(bool disposing)
| {
| if (Data != null)
| {
| Data.Length = 0;
| }
| }
|
| ~Vector1() //finalizer
| {
| Dispose(false);
| }
|


| public static implicit operator Vec1 (Vector1 AValue)
| {
| return AValue.Data;
| }

On return, the instance of Vector1 referenced by AValue becomes eligible for
collection (the JIT took care of that when loading the class), this is great
you don't need this instance any longer, you return a reference to a Vec1
instance, so far so good.
When the GC kicks-in, the Vector1 instance reference moves to the finalizer
queue, where it get's picked-up by the finalizer thread in order to run it's
finalizer, and here you set Data.Length=0... BINGO. Keep in mind that the
finalizer thread runs in // with your user code!!!


| public class Vec1Math
| {
| public static Vector1 Exp(Vec1 x)
| {
On entry, x holds a reference to a Vec1 instance no matter what......
| Vector1 vector = new Vector1(0);
| vector.Length = x.Length;
but he!, if the GC/finalizer ran before you reach this point, the value of
Length will be 0 (se above) and you will throw.

The reason for this all is an "incorrect" implementation of the Dispose
pattern, whenever you implement a finalizer you should take special care not
to create side effects when touching references co-owned by other object
instances. Implementing finalizers should be avoided at all costs, it's
realy hard to do correctly and in most cases it's not needed at all. Note
that I don't see any reason why you implement a disposable pattern, but I'm
affraid you say that this is only part of a 50 KLoc program :)

Willy.
 
B

Bill Butler

Atmapuri said:
That is not the reason. You can comment out GC.Collect and
the error still happens, but it is much harder to make it trigger.
You have to run it for extensive times and it will happen...

Did you happen to look at the sample program that I attached????
There is no call to GC.Collect...I cleaned up the problem down to it's
essence. The exception is quite reproducable.
I suggest you take my sample and play with it in order to convince
yourself.
The GC anyway keeps the reference count that references
the same object. According to the post of one of MVP's
the object reference is kept on the stack and that reference
will not be collected until the procedure ends.

I don't believe that your statement is accurate.
The GC starts from LIVE code and builds a tree of all live references.
Since the outer object is not referenced after the method call and it is
not passed into the method call, there is no LIVE reference to the outer
object....valid for GC.
So, the problem here is that the object itself is not collected
but its finalizer is called....

Incorrect, it was GCed because there were no live references to it.
It looks to me at least as a finalizer
bug.... What I need to know for certain is if this problem goes
100% away if Vector1 is a struct and the finalizer is moved
to the internal object.

Why do you NEED the finalizer????
Is there unmanaged resources in the REAL version of Vec1???
If not....stop doing it.
If so...consider the "using" paradigm.
As for your example, as soon as you have assigned the
object to local variable before passing it to the function, you
worked around the problem. For example.

I suggest you look at the sample I sent with Inner and Outer classes (my
second post)


temp = 2*c;
a = Func(temp);

Works fine.... But

a = Func(2*c)

fails.... But that seriously affects the use of expressions...
(Should not).

Both versions have the bug (in the finalizer)
The one version makes the problem more likely to occur, The other is a
timebomb.
If the GC kicks off during the method....Booom
Yes it absolutely does. A part from that, have you heard of
any major library that makes extensive use of overloaded
operators? There are two types of problems:

The overloaded operators are not the problem. They are simply triggering
the GC which exposes the real problem.

Take a look at the sample program.
Play with it.
Your finalizer is flawed.

Here it is again
Here is a short but complete program that demonstrates your problem
-------------------------
using System;
public class Inner
{
public int Value = 100;
}

public class Outer
{
public Inner Inner;

public Outer()
{
Inner = new Inner();
}

~Outer() {Inner.Value = 0;}
}

public class Test
{
public static void Main()
{
for (int i=0;i<=10000000;i++)
{
Outer outer = new Outer();
Func(outer.Inner); // since outer is not referenced again it is
fair game for GC
// outer.Inner.Value++; // This statement can fix the problem by
referencing outer
}
}

public static void Func(Inner Inner)
{
Outer outer = new Outer(); // this line triggers the GC to occur
eventually
if (Inner.Value == 0){ throw new Exception("Ooops"); }
}
}
 
A

Atmapuri

Hi!
realy hard to do correctly and in most cases it's not needed at all. Note
that I don't see any reason why you implement a disposable pattern, but
I'm
affraid you say that this is only part of a 50 KLoc program :)

:) Thanks for the detailed explanation. Now I understand...

Thanks!
Atmapuri
 
A

Atmapuri

Hi!

Looking to find a workaround:
Would using a struct instead of class make any difference
in that case? Would the temporary struct object also be immedatilly
invalidated by the garbage collector?

Are value types handled differently? The struct itself of course
can not have a finalizer, but an object inside of the struct
could have one that would implement a finalizer...

With struct I could not trigger an error in any case...and it appears
to work, but as I mentioned before, it has to work for sure...

Thanks!
Atmapuri
 
M

Marc Gravell

I don't think you're making it very clear what you are trying to work
around. From the disclosed information, the correct "workaround" is to ditch
the finalizers (not sure if you are still including these in your schemes).
To suggest an appropriate solution it is *essential* to understand broadly
what you are trying to do. For instance, disposable structs are a nightmare,
as they are so easily cloned (accidentally) - and then which one do you
dispose? double disposal (on different struct instances) would become a real
issue. Structs referencing an object will, when cloned, still reference the
same object instance, etc.

Ah, go on... tell us what you are trying to achieve...

Marc
 
W

Willy Denoyette [MVP]

| Hi!
|
| Looking to find a workaround:
| Would using a struct instead of class make any difference
| in that case? Would the temporary struct object also be immedatilly
| invalidated by the garbage collector?
|
| Are value types handled differently? The struct itself of course
| can not have a finalizer, but an object inside of the struct
| could have one that would implement a finalizer...
|
The object inside the struct would be an instance of Vec1 right? Well what
stops you to implement a finalizer in Vec1?? But again, what are you trying
to achieve, what would you ever need to do in Vec1's destructor???

a dirty hack meant to solve a problem which should'n even exist (a bug
introduced by YOU)... could look like this:

public class Vec1
{
internal Vector1 vRef; // store a reference to the containing instance
of Vector1
...

in Vector1.CreateFromCache you set the reference...
....
Data.vRef = this;
...



| With struct I could not trigger an error in any case...and it appears
| to work, but as I mentioned before, it has to work for sure...
|
| Thanks!
| Atmapuri
|
|

Please try to answer these simple questions:
why must the value (Length) be reset?
if you have a valid reason for this (but honestly I don't see any), why
are you relying on a non-deterministic destructor to do this? You are never
sure when it will be done, nor if it will ever be done.

The only workaround is - get rid of this Dispose pattern altogether -, you
don't need it, it put extreme high pressure on the GC which slows down your
code for no good reason (well, I don't see any).

Willy.
 
A

Atmapuri

Hi!
The object inside the struct would be an instance of Vec1 right?

I would implement another Vec1Container that would have the finalizer
and a pointer to Vec1 and Vector1 would only point to Vec1Container.
When struct Vector1 would be released from the stack, the finalizer
will get its chance..

I was thinking that struct's are allocated on the stack and not on the heap
and the stack is not cleared up until the expression has been evaluted.
This is not the same as for objects, where only the reference is stored
on the stack...
a dirty hack meant to solve a problem which should'n even exist (a bug
introduced by YOU)... could look like this:

public class Vec1
{
internal Vector1 vRef; // store a reference to the containing
instance
of Vector1

That would not work, because the finalizer would never get called,
because the Vec1 has a second reference that is never set to nil until
the end of app life.
Please try to answer these simple questions:
why must the value (Length) be reset?
if you have a valid reason for this (but honestly I don't see any), why
are you relying on a non-deterministic destructor to do this? You are
never
sure when it will be done, nor if it will ever be done.

With critical finalizers you are... Besides, my finalizer is very very
short.
That means that it will always be executed and if the app is destroyed
before that happens... no problem...
The only workaround is - get rid of this Dispose pattern altogether -, you
don't need it, it put extreme high pressure on the GC which slows down
your
code for no good reason (well, I don't see any).

The unmanaged resource is unmanaged memory. When you write expressions
you create a lot of temporary objects especially if you execute in a loop.
This gives great pressure on the garbage collector. One other problem
is that the garbage collector allocates memory over great spans of memory
which is not very close together and thus can not be cached by the CPU.

So, the idea is to allocate unmanaged memory and have that memory
preallocated
in a pool. Each time you allocate an object, it simply obtains a pointer
from the pool. Each time you free an object (via finalizer), the pointer
is released to the pool.

I was running some tests and due to tighter memory pool, the
performance gains of vectorized against plain function
for the function posted are around 5x and
that despite the fact that I call GC.Collect inside the
constructor every now and then...

The proper solution to this problem however would be to introduce
proper reference counted classes that have their destructors called
by the IL immediately after the function ends if their reference count
is 0. But this is only something that Microsoft can do...
..
Thanks!
Atmapuri
 
J

Jon Skeet [C# MVP]

Atmapuri said:
They are the "only" way to clean them up within expressions.

Then you shouldn't be allocating unmanaged resources within
expressions. Non-deterministic release of unmanaged resources is almost
always a bad idea. Implement IDisposable and use the resources in a
controlled way.

Not that I can see any unmanaged resources in your code in the first
place...
 
J

Jon Skeet [C# MVP]

Atmapuri said:
All the complaints about the way the code
is written have to do with missing code. What you dont
understand is not neccessary without meaning...

But you should listen to all the suggestions and consider that your
design is quite probably very flawed.
The CLR is managed and should not be failing if all
its objects are managed no matter what....

The CLR isn't failing. Your code is failing. Your finalizer is
modifying an object which is referenced elsewhere. The finalizer is
being called at a perfectly reasonable time - it's just that the
finalizer is doing unreasonable (or at least undesirable) things.
 
J

Jon Skeet [C# MVP]

The proper solution to this problem however would be to introduce
proper reference counted classes that have their destructors called
by the IL immediately after the function ends if their reference count
is 0. But this is only something that Microsoft can do...

See http://discuss.develop.com/archives/wa.exe?A2
=ind0010A&L=DOTNET&P=R28572

for an in-depth post about why .NET doesn't use reference counting.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Top