locked
Bug in .NET Framework 4.0? RRS feed

  • Question

  • UPDATE: 30 Aug, 2011
    there is no bug in .NET Framework. dispute is over.
    my opponent - PashaPash - has acknowledged that in some cases Dispose into using block boosts performance and there is no bug in my first example.


     

    Hello,

    MCC claims that the following code has a bug.

     

    class Entity : IDisposable
    {
     ~Entity() { }
     public virtual void Dispose() { GC.SuppressFinalize(this); }
    }
    class Collection : Entity
    {
     public IEnumerable Items { get; private set; }
     public Collection()
     {
     this.Items = new List();
     }
     public override void Dispose()
     {
     this.Items = null;
     base.Dispose();
     }
    }
    
    


    I believe there is no bug. Similar code we can find in .NET Framework. Samples are in theme (ru-ru).

    Please, review code and please, do not let them (mcc|moderators: PashaPash and Ulcer) remove theme!

    Thank you!

     

     




    • Edited by Malobukv Monday, August 29, 2011 10:58 PM
    Sunday, August 28, 2011 8:30 PM

All replies

  • if the items in the collection implement IDisposable (e.g. SqlConnection) the code would leak. I cannot read russian but the code posted by MCCs fixed the problem.

    This is not a bug in .Net, but a problem in your IDisposable implementation.

    BTW is there a reason not using generics interfaces and collections now?



    The following is signature, not part of post
    Please mark the post answered your question as the answer, and mark other helpful posts as helpful, so they will appear differently to other users who are visiting your thread for the same problem.
    Visual C++ MVP
    • Proposed as answer by ulcer Monday, August 29, 2011 11:32 AM
    • Unproposed as answer by Malobukv Monday, August 29, 2011 12:12 PM
    Sunday, August 28, 2011 8:49 PM
  • thank you.

    > if the items in the collection implement IDisposable (e.g. SqlConnection) the code would leak.

    there is no other code, just only what it is in the first message.
    how can I know about the leak? would you please tell me.

    > but the code posted by MCCs fixed the problem. 

    do you mean code under #1 in theme?
    it's not code of MCC. this is my code. but MCC claims about bug.

     

     

     



    Sunday, August 28, 2011 9:03 PM
  • If the code above is the complete code of the class, then there is no leak - the List class does not need finalizing and there is no item added to the list .

    But that would be an wasteful piece of code that does nothing beneficial to the end user. You won't see this in any project design, so the class would qualify as a design bug in real world. 



    The following is signature, not part of post
    Please mark the post answered your question as the answer, and mark other helpful posts as helpful, so they will appear differently to other users who are visiting your thread for the same problem.
    Visual C++ MVP
    Sunday, August 28, 2011 9:12 PM
  • Your implementation of IDisposable is incorrect.  It can cause a bug due to the virtual nature of how you have this implemented.

     

    In this case, there actually is no reason to implement IDisposable.  However, if you decide to implement it, I would recommend doing it correctly.

     

    For details on how (and when!) to do this correctly, see: http://reedcopsey.com/series/idisposable/


    Reed Copsey, Jr. - http://reedcopsey.com
    If a post answers your question, please click "Mark As Answer" on that post and "Mark as Helpful".
    Sunday, August 28, 2011 9:17 PM
    Moderator
  • thanks to all for answers. sorry fo my english.
    there is other example with special behavior.
    please, would you comment, why Clear makes execution more faster?
    and if there is a leak of memory, how i could know about?

    sorry, no formating. editor "eats" some characters

    using System;
    using System.Collections.Generic;
    using System.Diagnostics;
    using System.Drawing;
    using System.Windows.Forms;

    namespace WindowsFormsApplication12
    {
    public partial class Form1 : Form
    {
    List<int[]> arrs = new List<int[]>();
    void LoadArray()
    {
    var arr = new int[100000];
    arrs.Add(arr);
    }
    public Form1()
    {
    var count = 500.0;
    Button b1 = new Button() { Parent = this, Text = "create -> dispose", Location = new Point(10, 10), Width = 200 };
    b1.Click += (s, e) =>
    {
    var sw = Stopwatch.StartNew();
    arrs.Clear();
    for (var i = 0; i < count; i++)
    {
    using (var c = new Collection(0)) { }
    LoadArray();
    }
    System.Diagnostics.Trace.WriteLine(sw.ElapsedMilliseconds, b1.Text);
    };
    Button b2 = new Button() { Parent = this, Text = "create -> clear -> dispose", Location = new Point(10, b1.Bottom + 5), Width = 200 };
    b2.Click += (s, e) => // с Clear заметно быстрее
    {
    var sw = Stopwatch.StartNew();
    arrs.Clear();
    for (var i = 0; i < count; i++)
    {
    using (var c = new Collection(0))
    {
    c.Clear();
    }
    LoadArray();
    }
    System.Diagnostics.Trace.WriteLine(sw.ElapsedMilliseconds, b2.Text);
    };
    }
    class Entity : IDisposable
    {
    public int Id { get; private set; }
    public Entity(int id) { this.Id = id; }
    ~Entity() { }
    public virtual void Dispose() { GC.SuppressFinalize(this); }
    }
    class Collection : Entity
    {
    public IEnumerable<Entity> Items { get; private set; }
    public Collection(int id)
    : base(id)
    {
    var list = new List<Entity>();
    for (int i = 0; i < 10000; i++) list.Add(new Entity(i));
    this.Items = list;
    }
    public void Clear()
    {
    foreach (var itm in this.Items) itm.Dispose();
    ((List<Entity>)this.Items).Clear();
    }
    public override void Dispose()
    {
    this.Items = null;
    base.Dispose();
    }
    }
    }
    }

    Sunday, August 28, 2011 9:31 PM
  • You'll know there's a memory leak if your application keeps doing the same thing, and instead of memory being constant over a period of time, the amount of memory will increase.

    Remember, Dispose doesn't clean up memory, dispose is to be used release native resources. If you're class doesn't use P/Invoke to create any native objects, or has any disposable members, there's no need for the class to be disposable.

    Sunday, August 28, 2011 11:48 PM
  • > there's a memory leak if your application keeps doing the same thing

    how can I make sure that there is a memory leak in a code?
    should I use a windows task manager? but it is not accurate enough.

    > If you're class doesn't use P/Invoke to create any native objects, or has any disposable members, there's no need for the class to be disposable.

    i'm use IDisposable with using(...) { ... }

     

    Monday, August 29, 2011 7:06 AM
  • Malobukv, I already pointed you to a problem in the code above. It's not leaking memory or unmanaged resources (as there are not unmanaged resources used in this specific sample).

    However, there will be a possible resources "leak" in real application if Entity holds unmanaged resources.

    You sample has only a performance issue because you are not disposing (and SuppressFinalizing this.Items in Collection.Dispose(). Than causes an overload of F-Reachable queue. So the first sample will be definitely slower than second one, where you are explicitely calling Dispose/SuppressFinalizing.

    I already answered you this question about 10 times in Russian forums during last 5 days. Why do you need another "your implementation of IDisposable is incorrect" to believe?


    My blog | My Favorite Project
    Monday, August 29, 2011 1:54 PM
  • PashaPash> Malobukv, [...] in the code above. It's not leaking memory or unmanaged resources (as there are not unmanaged resources used inthis specific sample).

    that is, you agree that the code "as is" has no bug.


    REMINDER:  
    the code without bug was removed 3 times by PashaPash from http://social.msdn.microsoft.com/Forums/ru-RU/programminglanguageru/thread/aa78a422-c06f-4736-af43-20d048ee7a56#c7a18c3d-8ed2-4010-83bb-2ad251ca2ba7  moderators could see history of deletions. and there is comments of PashaPash with profanity words.

    Monday, August 29, 2011 2:34 PM
  • >> that is, you agree that the code "as is" has no bug.

    ok, it's not a bug. just serious performance issue.


    My blog | My Favorite Project
    Monday, August 29, 2011 2:57 PM
  • Making it disposable will only make your performance worse, because it now will take more steps for the GC to clean the object up.
    Monday, August 29, 2011 3:00 PM
  • > Making it disposable will only make your performance worse, because it now will take more steps for the GC to clean the object up

    ok. thank you.
    would you please explain why calling of Clear boosts performance?

    using (var c = new Collection(0))
    {
        c.Clear();
    }

    (see full code above in my post)

     

    Monday, August 29, 2011 4:28 PM
  • Obviously, you are not only clearing collection, but also explicitly disposing Entities in your Collection.Clear implementation.

    Clearing a collection itself has no significant impact on performance. Just try to remove the following line, and you will still have the same timings:

    ((List<Entity>)this.Items).Clear();


    My blog | My Favorite Project
    Monday, August 29, 2011 4:59 PM
  • > Just try to remove the following line, and you will still have the same timings:((List<Entity>)this.Items).Clear();

    try to remove something more :)

    could anybody explain performance boost in code "as is" with Clear, please.

    Monday, August 29, 2011 5:05 PM
  • We're not phsycic. We could only make stabs in the dark as to why clearing the collection could have performance improvements, given the little sample of code shown here. Without knowing what's in the collection, there's no way we could figure out why you perceive a performance improvement when the code clears the collection.
    Monday, August 29, 2011 6:45 PM
  • > given the little sample of code shown here


    sorry for my english. code is above in this theme.
    here is copy (without formating. because editor 'eats' some characters):

    using System;
    using System.Collections.Generic;
    using System.Diagnostics;
    using System.Drawing;
    using System.Windows.Forms;

    namespace WindowsFormsApplication12
    {
    public partial class Form1 : Form
    {
    List<int[]> arrs = new List<int[]>();
    void LoadArray()
    {
    var arr = new int[100000];
    arrs.Add(arr);
    }
    public Form1()
    {
    var count = 500.0;
    Button b1 = new Button() { Parent = this, Text = "create -> dispose", Location = new Point(10, 10), Width = 200 };
    b1.Click += (s, e) =>
    {
    var sw = Stopwatch.StartNew();
    arrs.Clear();
    for (var i = 0; i < count; i++)
    {
    using (var c = new Collection(0)) { }
    LoadArray();
    }
    System.Diagnostics.Trace.WriteLine(sw.ElapsedMilliseconds, b1.Text);
    };
    Button b2 = new Button() { Parent = this, Text = "create -> clear -> dispose", Location = new Point(10, b1.Bottom + 5), Width = 200 };
    b2.Click += (s, e) => // с Clear заметно быстрее
    {
    var sw = Stopwatch.StartNew();
    arrs.Clear();
    for (var i = 0; i < count; i++)
    {
    using (var c = new Collection(0))
    {
    c.Clear();
    }
    LoadArray();
    }
    System.Diagnostics.Trace.WriteLine(sw.ElapsedMilliseconds, b2.Text);
    };
    }
    class Entity : IDisposable
    {
    public int Id { get; private set; }
    public Entity(int id) { this.Id = id; }
    ~Entity() { }
    public virtual void Dispose() { GC.SuppressFinalize(this); }
    }
    class Collection : Entity
    {
    public IEnumerable<Entity> Items { get; private set; }
    public Collection(int id)
    : base(id)
    {
    var list = new List<Entity>();
    for (int i = 0; i < 10000; i++) list.Add(new Entity(i));
    this.Items = list;
    }
    public void Clear()
    {
    foreach (var itm in this.Items) itm.Dispose();
    ((List<Entity>)this.Items).Clear();
    }
    public override void Dispose()
    {
    this.Items = null;
    base.Dispose();
    }
    }
    }
    }

    Monday, August 29, 2011 7:16 PM
  • Ok, it will be even shorter: It's because you explicitly disposing Entities in your Collection.Clear implementation.

    My blog | My Favorite Project
    Monday, August 29, 2011 8:35 PM
  • PashaPash:
    Ok, it will be even shorter: It's because you explicitly disposing Entities in your Collection.Clear implementation.

    it's a part of answer. actualy into Entity.Dispose() is GC.SuppressFinalize - it's a key line.
    so, as I told you, in some cases we could use Dispose within using block for performance.

    nice that you understood.

    we spent a lot of efforts since 23 August, 2011 (7 days before) for this theme.
    please, never 
    use profanity words in argue and do not delete examples.

    thanks to all. we can close the theme.

    Best regards,
    Microsoft MVP C#, 2007-2011



    Monday, August 29, 2011 10:33 PM
  • Just a note for other answerers - this topic is one of many that Malobukv spawned to proove that clearing DataAdapter.TableMapping just before disposing an adatper will speed up the process. The original code could be seen in the first post of the topic he linked in post before. You don't need know russian to check that. Don't loose you time, Malobukv definitely know that DataTableMapping has no finalizer defined, and that his code is totally unrelated (yes, I told him few times). And yes, he known that there is no bug in .net 4.0. And yes, I told him that clearing an array itself is unrelated to performance.
    To "proove" that he is right, he typed some example with "Dispose" method named Clear, and broken Dispose. Then posted it in original topic stating that "Clearing a collection will speed up GC!, look on my code". And then he just used his moderator right to create illusion that his "sample" is somehow related to DataAdapter class. That's why this topic named Bug in .NET Framework 4.0.

    Then, eventually, his moderator rights was revoked. I also hope that his MVP status (if any existis, as I still does not known a real name of this "MVP") will be reviewed.

    Now he just translated an answer that I gave him 7 days before. Nice try, but anyone could use google translate to check that Malobukv is just lying: http://translate.google.com/translate?sl=auto&tl=en&u=http%3A%2F%2Fsocial.msdn.microsoft.com%2FForums%2Fru-RU%2Fvsru%2Fthread%2F2001ecc6-d241-439d-8b98-111b1eb67836. Check the post with translation starting with "A bit strange question".

    But both his and my posts was reviewed by Russian forum owners/moderators. And you can see the result - he moved to English part just becase got a final warinng "stop spamming with you broken sample, it prooves nothing" in russian .net forums. The only purpose of this topic was, definitely, to get an answer "that two MCC|moderators are wrong" from one of the English forums answerers. Final post should say to all of you "look, I told that PashaPash has no basic knowledge of GC!!!", hoping that no one of you know russian, and will have no chance to check.

    It's not a "Question Topic", just yet another spam "someone tell me that I right and bad Russian moderators are wrong topic". Sorry if you lost some time reading this topic from the beginning. Sorry for a long and probably offtopic answer. 

    My blog | My Favorite Project
    Monday, August 29, 2011 11:43 PM