Solved

destructor - override: memory management(again)

Posted on 2004-10-12
12
353 Views
Last Modified: 2013-12-03
So, this is basically a follow up to another question I posted earlier about memory management.

Consider the following classes:

TCell = class(TObject)
  status : cell_status;
  nA1, nA2 : integer;
  strand: integer;
  to_update : cell_status;
  lifetime: integer;
  resist: integer;
  activated: bool;
  constructor create();
end;

TMargolusBlock = class(TObject)
  cell : array[0..3] of TCell;
  cellCountH, cellCountA1, cellCountA2, cellCountA3, cellCountD : integer;
  LatticeReference1, LatticeReference2 : Integer;
  constructor create(cell0,cell1,cell2,cell3 : TCell);
  //destructor Destroy;
   // override;
end;

TMargolusArray = class(TObject)
  mblock : array of array of TMargolusBlock;
  constructor create(size : Integer);
  destructor Destroy;
    override;
end;

These are the constructors:

constructor TMargolusBlock.create(cell0,cell1,cell2,cell3 : TCell);
begin
  self.cell[0] := cell0;
  self.cell[1] := cell1;
  self.cell[2] := cell2;
  self.cell[3] := cell3;
  self.cellCountH := 0;
  self.cellCountA1 := 0;
  self.cellCountA2 := 0;
  self.cellCountA3 := 0;
  self.cellCountD := 0;
end;

constructor TMargolusArray.create(size : Integer);
begin
  setlength(mblock, size+1, size+1);
end;


Now, I tried using a custom destructor:

When I only use the second one it works(I think, I have some suspicions about memory usage/leakage)
but when I add the first one (Destructor TMargolusblock.Destroy) I get an error
Can anyone explain this to me?

{*
Destructor TMargolusblock.Destroy;
var
  Teller1 : Integer;
begin
    for Teller1 := 0 to 3 do
    begin
      cell[Teller1].Free();
    end;
   inherited destroy;
end;
*}

Destructor TMargolusArray.Destroy;
var
  Teller1, Teller2 : Integer;
begin
    for Teller1 := low(mblock[0]) to high(mblock[0]) do
    begin
      for Teller2 := low(mblock[0]) to high(mblock[0]) do
      begin
        mblock[Teller1, Teller2].Free();
      end;
    end;
   inherited destroy;
end;

This is when using another procedure to fill the MargolusArray.

After some debugging/testing I also noticed that this gives an error(related I guess):

constructor TMargolusArray.create(size : Integer);
var
  Teller1, Teller2 : Integer;
  empty_cell : TCell;
  empty_margolus : TMargolusBlock;
begin
  setlength(mblock, size+1, size+1);

  empty_cell := TCell.create;
  empty_margolus := TMargolusBlock.create(empty_cell,empty_cell,empty_cell,empty_cell);
  for Teller1 := 0 to size do
  begin
    for Teller2 := 0 to size do
    begin
      mblock[Teller1, Teller2] := empty_margolus;
    end;
  end;

Or even:
  mblock[5,5] := empty_margolus;
  mblock[5,6] := empty_margolus;

end;

I thought the mblock[index1,index2] was just a pointer to the empty_margolus memory location..?

Hope someone can explain it to me :)
0
Comment
Question by:reynaerde
  • 5
  • 4
  • 3
12 Comments
 
LVL 17

Expert Comment

by:geobul
ID: 12285897
Hi,

When you are defining a constructor you have to call 'inherited Create' as a first command in the constructor:

constructor TMargolusArray.create(size : Integer);
begin
  inherited Create;
  setlength(mblock, size+1, size+1);
end;

Regards, Geo
0
 
LVL 17

Assisted Solution

by:Wim ten Brink
Wim ten Brink earned 350 total points
ID: 12285903
I'm still amazed that "setlength(mblock, size+1, size+1);" just works. It should not work that way according to the Delphi helpfiles. :-)

Basically, your problem is simple... You create one object empty_cell and you're trying to free it four times!
When you assign an object to another variable then your not copying the object. Nope... You're just copying the reference, thus suddenly you have two variables pointing at the same object. The following code has the flaw:

  empty_cell := TCell.create;
  empty_margolus := TMargolusBlock.create(empty_cell,empty_cell,empty_cell,empty_cell);

Here, you create the object once, then pass it to the other object, which stores it in four different variables. Fortunately, you're not freeing empty_cell anywhere so it doesn't crash sooner... Try this instead:

  empty_margolus := TMargolusBlock.create(TCell.create,TCell.create,TCell.create,TCell.create);

Now, four cell objects are created and passed on. Which is better since you're going to free all four of them in your object too.

If you want to copy objects instead of copying references, then you should use TPersistent as base class instead so you can use the Assign method to assign the values of one object to the other. But this too has a flaw since TPersistent can only copy whatever data it knows itself, so you would need to override the Assign method, call the inherited Assign method first and then copy your own data from one object to the other.
0
 
LVL 17

Expert Comment

by:Wim ten Brink
ID: 12285916
@geobul, nope... Wrong :-P

If the base class is TObject then technically the only thing the inherited constructor does is zero the instance memory before it's used. Still, it is a very good idea to call the inherited Create method since it initializes data that you might need. It's just a bit sloppy if you don't...
0
 
LVL 17

Expert Comment

by:geobul
ID: 12285937
I'd use:

Destructor TMargolusblock.Destroy;
var
  Teller1 : Integer;
begin
    for Teller1 := 3 downto 0 do
    begin
      cell[Teller1].Free();
    end;
   inherited destroy;
end;

Regards, Geo
0
 
LVL 17

Expert Comment

by:geobul
ID: 12285961
I had in mind:

Destructor TMargolusblock.Destroy;
var
  Teller1 : Integer;
begin
    for Teller1 := 3 downto 0 do
    begin
      cell[Teller1] := nil; // not Free
    end;
   inherited destroy;
end;

Sorry.
0
 

Author Comment

by:reynaerde
ID: 12285982
Ok, but:

- I thought free() checks for nil reference anyway
- Why can't I assign two variables pointing at the same object?

constructor TMargolusArray.create(size : Integer);
var
  empty_cell : TCell;
  empty_margolus : TMargolusBlock;
begin
  setlength(mblock, size+1, size+1);

  empty_margolus := TMargolusBlock.create(TCell.create,TCell.create,TCell.create,TCell.create);

  mblock[5,5] := empty_margolus;
  mblock[5,6] := empty_margolus;

end;

This code gives me an error, WITHOUT using any destructor..
0
IT, Stop Being Called Into Every Meeting

Highfive is so simple that setting up every meeting room takes just minutes and every employee will be able to start or join a call from any room with ease. Never be called into a meeting just to get it started again. This is how video conferencing should work!

 

Author Comment

by:reynaerde
ID: 12286092
I'm sorry, I stand corrected on the last point, there was another error in my code causing the error.
0
 
LVL 17

Assisted Solution

by:geobul
geobul earned 150 total points
ID: 12286094
Well, your approach confuses me because I'd never do it this way.

1. TMargolusBlock contains array of TCell objects, so I'd create them in TMargolusBlock.Create and free them in TMargolusBlock.Destroy.

constructor TMargolusBlock.create;
var i: integer;
begin
  inherited Create; // Alex said it's not necessary and I believe him ;-)
  for i := 0 to 3 do Cell[i].Create;
  self.cellCountH := 0;
  self.cellCountA1 := 0;
  self.cellCountA2 := 0;
  self.cellCountA3 := 0;
  self.cellCountD := 0;
end;

destructor TMargolusBlock.Destroy;
var i: integer;
begin
  for i := 3 downto 0 do Cell[i].Free;
  inherited Destroy;
end;

2. The same for TMargolusArray: it contains array of TMargolusBlock objects, so use TMargolusArray.Create to create them and TMargolusArray.Destroy to free them.

Finally, in order to use all this you have to call:

var
  ma: TMargolusArray;
  size: integer;
begin
  size := 5;
  ma := TMargolusArray.Create(size);
  try
    .....
  finally
    ma.Free;
  end;
end;

Regards, Geo
0
 

Author Comment

by:reynaerde
ID: 12286412
That is more or less what I'm trying to do, only the problem is:

* I need to be able to have different cells in the MargolusBlock point to the same cell AND free the memory afterwards..
For example, in my program I will have:

(not real code, just to give an idea)
MA1, MA2 : TMargolusArray;
cellA : TCell;

where:

cellA := TCell.create();
MA1 := TMargolusArray.create(10);
MA2 := TMargolusArray.create(10);

MA1.mblock[1, 2] := TMargolusBlock.create(cellA,cellX1,cellX2,cellX3);
MA2.mblock[2, 2] := TMargolusBlock.create(cellA,cellY1,cellY2,cellY3);

Using this constructor:

constructor TMargolusBlock.create(cell0,cell1,cell2,cell3 : TCell);
begin
  self.cell[0] := cell0;
  self.cell[1] := cell1;
  self.cell[2] := cell2;
  self.cell[3] := cell3;
  self.cellCountH := 0;
  self.cellCountA1 := 0;
  self.cellCountA2 := 0;
  self.cellCountA3 := 0;
  self.cellCountD := 0;
end;

So, instead of creating a new TCell, I'm just referencing an already existing TCell.

The assignment part works fine, only when it comes to freeing it afterwards I run into problems..
Is there any way to have this working? Or should I really make a copy of each TCell?? This would mean a LOT of extra work since I'm accessing the TCells through a number of different Objects(not just TMargolusArrays, but also TLattice, another class which has an array of cells in it)

Thanks a bunch in advance!




0
 
LVL 17

Assisted Solution

by:geobul
geobul earned 150 total points
ID: 12286945
If you don't create TCell objects in TMargolusBlock.create then you don't have to free them in the destructor (or even set them to nil). You should free the cells in the object methods (or other code) where you create them. As simple as that.
0
 
LVL 17

Accepted Solution

by:
Wim ten Brink earned 350 total points
ID: 12287671
Well, the problem is that he would have to free those Cell objects somewhere. If he doesn't create them in his objects and he doesn't keep a reference to them, they just end up in limbo, never to be freed again. This is why I prefer working with interfaces these days. You assign an object to an interface and just keep passing that interface around. As long as one single piece of code is referring to it, it exists. Once the last reference is gone, the object in the interface is automatically freed.
But interfaces are more advanced stuff..

And the difference between
 for i := 3 downto 0 do Cell[i].Free;
and
 for i := 0 to 3 do Cell[i].Free;
???
Well, in this case there isn't any. Going in reverse order is only useful when deleting items from a list.

- I thought free() checks for nil reference anyway
Yes. And it will generate an exception if it's not assigned. But even worse, you do free the objects but don't set the array elements to nil, so it's never nil. Even if you would use:
 for i := 0 to 3 do begin
    Cell[i].Free;
    Cell[i] := nil;
  end;
This would still fail because you set Cell[0] to nil but Cell[1] is still pointing to where your object used to be.

- Why can't I assign two variables pointing at the same object?
Well, you can. But they would both be pointing to the same object. It won't generate two copies of the object. Thus, if you free one of them then you cannot free the other.
You could use this code, though:

  if Assigned( Cell[ 0 ] ) and ( Cell[ 0 ] <> Cell[ 1 ] ) and ( Cell[ 0 ] <> Cell[ 2 ] ) and ( Cell[ 0 ] <> Cell[ 3 ] ) then Cell[ 0 ].Free;
  if Assigned( Cell[ 1 ] ) and ( Cell[ 1 ] <> Cell[ 2 ] ) and ( Cell[ 1 ] <> Cell[ 3 ] ) then Cell[ 1 ].Free;
  if Assigned( Cell[ 2 ] ) and ( Cell[ 2 ] <> Cell[ 3 ] ) then Cell[ 2 ].Free;
  if Assigned( Cell[ 3 ] ) then Cell[ 3 ].Free;

This would check if you don't have multiple cells pointing at the same instance. But it's just better to give each variable it's own instance. (Or better, use interfaces, but that's way more complicated.) Above code would become quite complicated if this array had 50 elements instead of 4...

I've had to do something similar like this, maintaining arrays of arrays of data but I decided to make the arrays of interfaces instead of arrays of objects. I'll give you a simple example here, but it will probably confuse you more... ;-)

type
  IDevice = interface( IUnknown )
    function Equals( Value: IDevice ): Boolean; overload; safecall;
    function Equals( Value: TDeviceID ): Boolean; overload; safecall;
    function GetDeviceStatus: TDeviceStatus; safecall;
    function GetDisplayName: string; safecall;
    function GetDisplayNumber: string; safecall;
    function GetDND: Boolean; safecall;
    function GetForwarding: string;
    function GetHandle: TDeviceID; safecall;
    function GetName: string; safecall;
    function GetNumber: string; safecall;
    function GetPhoneLocation: TPhoneLocation; safecall;
    function GetPhoneType: TPhoneType; safecall;
    function HasDeviceStatus( Status: TDeviceStatusSet ): Boolean;
    procedure SetDeviceStatus( Value: TDeviceStatus ); safecall;
    procedure SetDisplayName( const Value: string ); safecall;
    procedure SetDisplayNumber( const Value: string ); safecall;
    procedure SetDND( Value: Boolean ); safecall;
    procedure SetForwarding( Value: string );
    property DeviceStatus: TDeviceStatus read GetDeviceStatus write SetDeviceStatus;
    property DisplayName: string read GetDisplayName write SetDisplayName;
    property DisplayNumber: string read GetDisplayNumber write SetDisplayNumber;
    property DND: Boolean read GetDND write SetDND;
    property Forwarding: string read GetForwarding write SetForwarding;
    property Handle: TDeviceID read GetHandle;
    property Name: string read GetName;
    property Number: string read GetNumber;
    property PhoneLocation: TPhoneLocation read GetPhoneLocation;
    property PhoneType: TPhoneType read GetPhoneType;
  end;
  TDevice = class( TInterfacedObject, IDevice )
  private
    FDeviceStatus: TDeviceStatus;
    FDisplayName: string;
    FDisplayNumber: string;
    FDND: Boolean;
    FForwarding: string;
    FIndex: TDeviceID;
    FNumber: string;
    FPhoneLocation: TPhoneLocation;
    FPhoneType: TPhoneType;
  protected
    function Equals( Value: IDevice ): Boolean; overload; safecall;
    function Equals( Value: TDeviceID ): Boolean; overload; safecall;
    function GetDeviceStatus: TDeviceStatus; safecall;
    function GetDisplayName: string; safecall;
    function GetDisplayNumber: string; safecall;
    function GetDND: Boolean; safecall;
    function GetForwarding: string;
    function GetHandle: TDeviceID; safecall;
    function GetName: string; safecall;
    function GetNumber: string; safecall;
    function GetPhoneLocation: TPhoneLocation; safecall;
    function GetPhoneType: TPhoneType; safecall;
    function HasDeviceStatus( Status: TDeviceStatusSet ): Boolean;
    procedure SetDeviceStatus( Value: TDeviceStatus ); safecall;
    procedure SetDisplayName( const Value: string ); safecall;
    procedure SetDisplayNumber( const Value: string ); safecall;
    procedure SetDND( Value: Boolean ); safecall;
    procedure SetForwarding( Value: string );
  public
    constructor Create;
    destructor Destroy; override;
  end;

There's a lot of code going on behind those methods, though. I won't bother you with that, though. However, above interface allows you to do things like this:

var
  Device: IDevice; // IDevice, not TDevice!!! I, not T.
  DeviceList: array[0..9] of IDevice;
  I: Integer;
begin
  Device := TDevice.Create;
  // Now you can access all methods and properties of IDevice, but not TDevice!
  for I := Low(DeviceList) to High(DeviceList) do // Low() and High() are more flexible.
  begin
    DeviceList[I] := Device;
  end;
  // But you can also do this:
  DeviceList[5] := TDevice.Create;
  And do you need to free it? Nope. As soon as DeviceList gets out of scope, it will clear the array. But still, this code is more safe:
  for I := Low(DeviceList) to High(DeviceList) do DeviceList[I] := nil;
  // Notice I don't call the Free method. I just assign nil.
  // However, at this point the Device isn't freed yet since the Device variable is also still linking to it! :-)
  Device := nil;
  // Now the device is completely freed.
end;
0
 

Author Comment

by:reynaerde
ID: 12287777
Well, I finally managed to bring my memory leak down from something like 20MB/100 iterations to around 10 bytes. Still not perfect but good enough for me. I still don't feel I really understand the different aspects of memory management but certainly feel I have a better grasp.
So, thank you both a lot. I hope you feel the division of points is a fair one.
Thanks again!
0

Featured Post

Free Trending Threat Insights Every Day

Enhance your security with threat intelligence from the web. Get trending threat insights on hackers, exploits, and suspicious IP addresses delivered to your inbox with our free Cyber Daily.

Join & Write a Comment

A lot of questions regard threads in Delphi.   One of the more specific questions is how to show progress of the thread.   Updating a progressbar from inside a thread is a mistake. A solution to this would be to send a synchronized message to the…
Objective: - This article will help user in how to convert their numeric value become words. How to use 1. You can copy this code in your Unit as function 2. than you can perform your function by type this code The Code   (CODE) The Im…
Excel styles will make formatting consistent and let you apply and change formatting faster. In this tutorial, you'll learn how to use Excel's built-in styles, how to modify styles, and how to create your own. You'll also learn how to use your custo…
When you create an app prototype with Adobe XD, you can insert system screens -- sharing or Control Center, for example -- with just a few clicks. This video shows you how. You can take the full course on Experts Exchange at http://bit.ly/XDcourse.

760 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

22 Experts available now in Live!

Get 1:1 Help Now