Lazarus

Free Pascal => General => Topic started by: CyberFilth on May 30, 2021, 12:36:56 pm

Title: [Solved] Trying to find a memory leak with no objects
Post by: CyberFilth on May 30, 2021, 12:36:56 pm
I'm working on a small game that runs in the terminal and noticed that under certain conditions there's a memory leak.
I'm used to leaks from unfreed objects in GUI applications but this game doesn't use objects. The problem seems to be related to a dynamic array of records.

The stack trace occurs when removing something from the players inventory and dropping it on the ground.
The stack trace is here

Quote
Heap dump by heaptrc unit of /home/cyberfilth/Development/ASCII-axe/ascii_axe
163037 memory blocks allocated : 21608896/22086680
163035 memory blocks freed     : 21608805/22086584
2 unfreed memory blocks : 91
True heap size : 688128
True free heap : 687584
Should be : 687648
Call trace for block $00007FC14DD568E0 size 52
  $000000000047259A  DROPSELECTION,  line 266 of player/player_inventory.pas
  $000000000045BA29  DROPINPUT,  line 196 of keyboardinput.pas
  $000000000045B749  WAITFORINPUT,  line 87 of keyboardinput.pas
  $000000000047253C  DROP,  line 259 of player/player_inventory.pas
  $000000000045BE68  GAMEINPUT,  line 368 of keyboardinput.pas
  $000000000045B7C1  WAITFORINPUT,  line 108 of keyboardinput.pas
  $0000000000429281  INITIALISE,  line 82 of main.pas
  $000000000040109E  main,  line 24 of ascii_axe.lpr
Call trace for block $00007FC14DCCD200 size 39
  $000000000047259A  DROPSELECTION,  line 266 of player/player_inventory.pas
  $000000000045BA29  DROPINPUT,  line 196 of keyboardinput.pas
  $000000000045B749  WAITFORINPUT,  line 87 of keyboardinput.pas
  $000000000047253C  DROP,  line 259 of player/player_inventory.pas
  $000000000045BE68  GAMEINPUT,  line 368 of keyboardinput.pas
  $000000000045B7C1  WAITFORINPUT,  line 108 of keyboardinput.pas
  $0000000000429281  INITIALISE,  line 82 of main.pas
  $000000000040109E  main,  line 24 of ascii_axe.lpr
  $000000000040109E  main,  line 24 of ascii_axe.lpr

The procedure it points to is just

Code: Pascal  [Select][+][-]
  1. procedure dropSelection(selection: smallint);
  2. begin
  3.   (* Check that the slot is not empty *)
  4.   if (inventory[selection].inInventory = True) then
  5.   begin
  6.     if (removeFromInventory(selection) = True) then
  7.       ui.displayMessage('Successful dropped message');
  8.   end
  9.   else
  10.     ui.displayMessage('This slot is empty message');
  11. end;

Which calls the following function

Code: Pascal  [Select][+][-]
  1. function removeFromInventory(itemNumber: smallint): boolean;
  2. var
  3.   newItem: item;
  4. begin
  5.   Result := False;
  6.   (* Check if there is already an item on the floor here *)
  7.   if (items.containsItem(entityList[0].posX, entityList[0].posY) = False) then
  8.     { Create an item }
  9.   begin
  10.     newItem.itemID := items.itemAmount;
  11.     newItem.itemName := inventory[itemNumber].Name;
  12.     newItem.itemDescription := inventory[itemNumber].description;
  13.     newItem.itemType := inventory[itemNumber].itemType;
  14.     newItem.itemMaterial := inventory[itemNumber].itemMaterial;
  15.     newItem.useID := inventory[itemNumber].useID;
  16.     newItem.glyph := inventory[itemNumber].glyph;
  17.     newItem.glyphColour := inventory[itemNumber].glyphColour;
  18.     newItem.inView := True;
  19.     newItem.posX := entities.entityList[0].posX;
  20.     newItem.posY := entities.entityList[0].posY;
  21.     newItem.onMap := True;
  22.     newItem.discovered := True;
  23.  
  24.     { Place item on the game map }
  25.     Inc(items.itemAmount);
  26.     Insert(newitem, itemList, itemAmount);
  27.     ui.bufferMessage('You drop the ' + newItem.itemName);
  28.  
  29.     (* Remove from inventory *)
  30.     inventory[itemNumber].Name := 'Empty';
  31.     inventory[itemNumber].equipped := False;
  32.     inventory[itemNumber].description := 'x';
  33.     inventory[itemNumber].itemType := itmEmptySlot;
  34.     inventory[itemNumber].itemMaterial := matEmpty;
  35.     inventory[itemNumber].glyph := 'x';
  36.     inventory[itemNumber].glyphColour := 'x';
  37.     inventory[itemNumber].inInventory := False;
  38.     inventory[itemNumber].useID := 0;
  39.     Result := True;
  40.     (* Redraw the Drop menu *)
  41.     drop;
  42.   end
  43.   else
  44.     ui.bufferMessage('There is no room here');
  45. end;
         

Any suggestions on what could be causing this leak? The full code is at https://github.com/cyberfilth/ASCII-axe
Title: Re: Trying to find a memory leak with no objects
Post by: MarkMLl on May 30, 2021, 01:00:37 pm
How are items created and destroyed: are they on the heap? Are you 100% confident in your handling of inInventory? Are you being defensive about potentially overwriting an already-existing item in a slow, and making sure that a slot is cleared when one is destroyed?

MarkMLl
Title: Re: Trying to find a memory leak with no objects
Post by: CyberFilth on May 30, 2021, 03:00:32 pm
As I understand it, these are global records so should be created on the heap.
No record is ever really deleted and no slots are destroyed. The items on the map are given the name 'Empty' when copying their details to the inventory, then when the game is saved, any item with the name 'Empty' is ignored and not saved.

Items are added to the game map by creating a new entry in an array called itemList and adding them.

I've looked for the usual suspects for what could be causing unfreed memory. But since I'm not using images, objects, stringlists or pointers in the relevant section of code I'm stumped what could be causing it.
Title: Re: Trying to find a memory leak with no objects
Post by: Martin_fr on May 30, 2021, 03:24:53 pm
Not enough code to say....

But it can be that "dropSelection" adds something (like an ansistring/ dyn array) to an allocated object.
If that allocated object is not freed, then neither is the string => so the string is a leak, but the stack for it is not helpful.

However then you should have the leak of the object holding this date.

---
Btw: menu View > View leaks and traces

Paste the leak output, and you can navigate in your sources by double-click.



Title: Re: Trying to find a memory leak with no objects
Post by: engkin on May 30, 2021, 07:23:07 pm
The leak seems to be caused by the + operator. I did the following changes, and the problem disappeared:

In unit ui,
change buffer to shortstring
change displayMessage to use shortstring
change bufferMessage to use shortstring

In unit player_inventory,
removeFromInventory function, instead of:
Code: Pascal  [Select][+][-]
  1.   ui.bufferMessage('You drop the '+newItem.itemName);
use:
Code: Pascal  [Select][+][-]
  1.   WriteStr(ss,'You drop the ',newItem.itemName);
  2.   ui.bufferMessage(ss);
ss is a local var of shortstring

Title: Re: Trying to find a memory leak with no objects
Post by: CyberFilth on May 30, 2021, 09:22:48 pm
engkin that's amazing, thanks so much for taking the time to go through that.

It wouldn't have occurred to me that appending to a string like that could cause an issue. I'm going to have to go through the rest of my code now and see if I can catch it happening somewhere else.

Thanks again
Title: Re: Trying to find a memory leak with no objects
Post by: MarkMLl on May 30, 2021, 09:39:20 pm
Quote from: CyberFilth on May 30, 2021, 09:22:48 pm
engkin that's amazing, thanks so much for taking the time to go through that.

It wouldn't have occurred to me that appending to a string like that could cause an issue. I'm going to have to go through the rest of my code now and see if I can catch it happening somewhere else.

Thanks again

It's certainly an interesting one, and suggests that a lot more of us should probably be using heaptrc on apparently-correct programs.

MarkMLl
Title: Re: Trying to find a memory leak with no objects
Post by: CyberFilth on June 01, 2021, 03:14:18 pm
Quote from: engkin on May 30, 2021, 07:23:07 pm
The leak seems to be caused by the + operator. I did the following changes, and the problem disappeared:

In unit ui,
change buffer to shortstring
change displayMessage to use shortstring
change bufferMessage to use shortstring

In unit player_inventory,
removeFromInventory function, instead of:
Code: Pascal  [Select][+][-]
  1.   ui.bufferMessage('You drop the '+newItem.itemName);


use:
Code: Pascal  [Select][+][-]
  1.   WriteStr(ss,'You drop the ',newItem.itemName);
  2.   ui.bufferMessage(ss);
ss is a local var of shortstring

Can I ask how you found the problem? I'm wondering if it's a known issue.
Title: Re: [Solved] Trying to find a memory leak with no objects
Post by: engkin on June 01, 2021, 05:14:10 pm
I think it is not known, and needs to be reported. It would need the smallest possible test project to help the developers work on it.

Thank you, I did enjoy playing the game to hunt this bug. The heap tracker pointed at the correct location, almost. Your code does not reserve any memory other than the dynamic arrays. I did spot the mixed usage of strings and shortstrings. That made them the prime suspect. commenting out the messaging function got rid of the problem.

So you could say it was luck and enjoyment  :D
Title: Re: [Solved] Trying to find a memory leak with no objects
Post by: engkin on June 01, 2021, 05:32:46 pm
I saw the leak using Laz2.0.10/FPC3.2.0/Win32

Search for an item (yes, play the game)
Grab the item (using G)
Drop it using (using D) <--- this causes the leak
Quit the game (ESC then Q) <--- see the leak

I pointed the heap tracker to a file.
Title: Re: [Solved] Trying to find a memory leak with no objects
Post by: engkin on June 02, 2021, 04:51:23 am
@CyberFilth, I think you have a logical problem that caused this leak.
Your code enters removeFromInventory and it never exits it. You probably do the same in other parts of your program.

The compiler releases the temporary memory it reserves at the end of the  procedure. If the code never reaches the end, like in this case, the memory will not get released.

function removeFromInventory
begin
...
  the compiler reserves some temporary memory for the + operator
..
  drop<--- this call never ends
...
  the compiler releases the memory <--- this does not get called
end;
Title: Re: [Solved] Trying to find a memory leak with no objects
Post by: CyberFilth on June 02, 2021, 09:22:49 am
Ah, that makes sense. I've been using main.gameState to control the flow of the program, so whenever keyboardInput.waitForInput is called it checks the current game state and then calls the relevant procedure.

As you noted before, the game doesn't reserve any other memory so this hadn't caused an issue before now. The GUI version of the game calls a procedure when exiting the game that explicitly frees all images loaded at startup.
Title: Re: [Solved] Trying to find a memory leak with no objects
Post by: engkin on June 02, 2021, 03:07:33 pm
Create a new procedure in "main" unit:
Code: Pascal  [Select][+][-]
  1. ...
  2. procedure loop;
  3.  
  4. implementation
  5.  
  6. uses
  7.   ..., Keyboard;
  8.  
  9. ...
  10. procedure loop;
  11. var
  12.   Keypress: TKeyEvent;
  13. begin
  14.   while true do
  15.   begin
  16.     Keypress := GetKeyEvent;
  17.     Keypress := TranslateKeyEvent(Keypress);
  18.     case gameState of
  19.       { ---------------------------------   Title menu }
  20.       stTitle:titleInput(Keypress);
  21.       { ------------------------------------  Game Over screen }
  22.       stGameOver:RIPInput(Keypress);
  23.       { ----------------------------------     Prompt to quit game }
  24.       stQuitMenu:quitInput(Keypress);
  25.       { ---------------------------------    In the Inventory menu }
  26.       stInventory:inventoryInput(Keypress);
  27.       { ---------------------------------    In the Drop item menu }
  28.       stDropMenu:dropInput(Keypress);
  29.       { ---------------------------------    In the Quaff menu }
  30.       stQuaffMenu:quaffInput(Keypress);
  31.       { ---------------------------------    In the Wear / Wield menu }
  32.       stWearWield:wearWieldInput(Keypress);
  33.       { ---------------------------------    Gameplay controls }
  34.       stGame:
  35.         gameInput(Keypress);
  36.     end;//case
  37.   end;//while
  38. end;

call it after main.initialise in the project file:
Code: Pascal  [Select][+][-]
  1.   main.initialise;
  2.   main.loop;

the loop procedure is replacing keyboardinput.waitForInput.
Comment out or delete the code inside keyboardinput.waitForInput:
Code: Pascal  [Select][+][-]
  1. procedure WaitForInput;
  2. begin
  3. end;

Test the game, and clean up the code by removing  keyboardinput.waitForInput and every call to it.
Title: Re: [Solved] Trying to find a memory leak with no objects
Post by: CyberFilth on June 02, 2021, 04:07:46 pm
That's so much simpler than the procedure that I was using!
I really appreciate the time that you've taken on this, especially picking your way through my code which is... inelegant, to put it mildly.
Reading and making sense of other peoples code takes me a long time.

I've implemented the changes that you posted and it works.
Thanks again for all of your help.
TinyPortal © 2005-2018