T_X uploaded file valgrind-suppresions (2.1 KiB)

Valgrind suppresions file to exclude external libraries

T_X uploaded file valgrind.log (77.5 KiB)

Valgrind log output for warzone2100 (excluding libraries)

vexed commented

I have fixed some of these already, the Net* routines are the main source of leaks from my testing.

The NetQueue::pushMessage, is one of the big issues, and needs to be fixed.
For what it is worth, my results for my game are:

detected 747486 memory leaks (45514706 bytes).
Largest number used: 68394554 bytes.
Total allocations: 3133883019 bytes.

Not sure if I should keep this one open, or mark it a duplicate of #3218

T_X commented

Replying to Warzone2100/old-trac-import#3395 (comment:1):

I have fixed some of these already, the Net* routines are the main source of leaks from my testing.

The NetQueue::pushMessage, is one of the big issues, and needs to be fixed.
For what it is worth, my results for my game are:

detected 747486 memory leaks (45514706 bytes).
Largest number used: 68394554 bytes.
Total allocations: 3133883019 bytes.

Aiy, okay, great :). Do you have a public branch for the things you've fixed already somewhere? (Couldn't spot them in the master or bugfixes branches or on your personal github profile.) I gave some easy ones a try myself here: Warzone2100/warzone2100#30

Not sure if I should keep this one open, or mark it a duplicate of #3218

I don't mind, I can also open new, more specific tickets for still remaining issues later.

vexed changed priority from normal to major

vexed changed milestone from unspecified to 3.1

vexed commented

Replying to Warzone2100/old-trac-import#3395 (comment:2):

Replying to Warzone2100/old-trac-import#3395 (comment:1):

I have fixed some of these already, the Net* routines are the main source of leaks from my testing.

The NetQueue::pushMessage, is one of the big issues, and needs to be fixed.
For what it is worth, my results for my game are:

detected 747486 memory leaks (45514706 bytes).
Largest number used: 68394554 bytes.
Total allocations: 3133883019 bytes.

Aiy, okay, great :). Do you have a public branch for the things you've fixed already somewhere? (Couldn't spot them in the master or bugfixes branches or on your personal github profile.) I gave some easy ones a try myself here: Warzone2100/warzone2100#30

No, I don't have a public branch with the fixes that I have done.
A quick look at your patches, and you have one that I didn't fix, the other ones I have fixed another way. Still testing these as well.

I'll attach the patches here, though, I am still not done with all of them.

Not sure if I should keep this one open, or mark it a duplicate of #3218

I don't mind, I can also open new, more specific tickets for still remaining issues later.

vexed uploaded file 0002-Cleanup-leak-in-effects.patch (0.9 KiB)

vexed uploaded file 0006-Cleanup-some-viewdata-leaks.patch (3.1 KiB)

vexed commented

More patches in #3399 as well, though, only http://developer.wz2100.net/attachment/ticket/3399/0004-Fix-non-critical-memleak-in-createHeader-dbgHeader.patch

is the one that I didn't get around to fixing yet.

vexed commented

These are more areas where it leaks...

warzone2100\src\astar.cpp (531): Warzone2100-Dbg.exe!fpathAStarRoute + 0x16 bytes

warzone2100\lib\script\event.cpp (428): Warzone2100-Dbg.exe!eventNewContext + 0xD bytes

warzone2100\lib\script\event.cpp (367): Warzone2100-Dbg.exe!eventNewContext + 0x18 bytes

warzone2100\lib\script\script_parser.ypp (1876): Warzone2100-Dbg.exe!scr_parse + 0x29 bytes

warzone2100\src\droid.cpp (1836): Warzone2100-Dbg.exe!reallyBuildDroid + 0xA bytes

warzone2100\src\visibility.cpp (302): Warzone2100-Dbg.exe!visTilesUpdate + 0x12 bytes

warzone2100\src\order.cpp (2205): Warzone2100-Dbg.exe!orderDroidAddPending

T_X commented

Awesome! Had just finished the pushMessage and gameQueue leak, too (before checking the timeline here and noticing that you had already done that :/ ).

The NetQueue::pushMessage, is one of the big issues, and needs to be fixed.

This one didn't seem that serious to me anymore, I think this memory leak shouldn't grow with every game round. I think the message list just didn't get freed due to the various networking queues not being freed (but my C++/STL knowledge is rather rudimentary, feel free to correct me :) ). http://developer.wz2100.net/attachment/ticket/3399/0003-Fix-non-critical-memleak-in-NETinitQueue-netQueues-a.patch and http://developer.wz2100.net/attachment/ticket/3399/0005-Fix-non-critical-memleak-in-NETinitQueue-netGameQueu.patch seemed to work, are you sure the explicit .clear() methods / destructors you've added in http://developer.wz2100.net/attachment/ticket/3395/0001-Cleanup-some-leaks-in-the-NET-routines.patch are really necessary?

Hmm, yeah, MAX_PLAYERS and MAX_CONNECTED_PLAYERS are currently defined as the same thing. But still, the NETinitQueue() for the game queues uses MAX_CONNECTED_PLAYERS. If someone were later going to change MAX_PLAYERS != MAX_CONNECTED_PLAYERS, there might be unnoticed surprises. What do you think, would it make sense to either have two for-loops with MAX_CONNECTED_PLAYERS and MAX_PLAYERS each in NETdeleteQueue() or otherwise simply removing one of these defines?

From http://developer.wz2100.net/attachment/ticket/3395/0002-Cleanup-leak-in-effects.patch:

free(chunkList.first);

Are you sure that only deleting the head of this list is sufficient? Compare with http://developer.wz2100.net/attachment/ticket/3399/0002-Fix-non-critical-memleak-in-initEffectsSystem-chunkL.patch

shutdownEffectsSystem(); // Clean up old chunks

If I'm not missing something then this shouldn't be necessary due to both initEffectsSystem() and systemShutdown() already calling shutdownEffectsSystem(), right? But I agree, it looks more sane having the effects system cleanup in the stageThreeShutDown() path instead of having them in initialization code paths. What do you think, would it make sense to either remove the shutdownEffectsSystem() you added for now (and maybe refactor this later, also other parts of the code seem to have mixed ways of doing the init and freeing stuff)? Or otherwise removing the shutdownEffectsSystem() from initEffectsSystem()/systemShutdown() in your patch as well?

For the other things, you fixed, nice, too! Didn't have a chance to look at them or test them in detail yet though. But I'm excited to see how few errors will be left in valgrind :).

PS: Out of curiousity, how did you produce your memory leak reports? Also, how many game rounds did you start and how long did you play to produce these quite a bit larger amounts of lost bytes?

vexed commented

Replying to Warzone2100/old-trac-import#3395 (comment:6):

Awesome! Had just finished the pushMessage and gameQueue leak, too (before checking the timeline here and noticing that you had already done that :/ ).

The NetQueue::pushMessage, is one of the big issues, and needs to be fixed.

This one didn't seem that serious to me anymore, I think this memory leak shouldn't grow with every game round. I think the message list just didn't get freed due to the various networking queues not being freed (but my C++/STL knowledge is rather rudimentary, feel free to correct me :) ).

It grew over time, and I suppose if you did back-2-back games as well.

http://developer.wz2100.net/attachment/ticket/3399/0003-Fix-non-critical-memleak-in-NETinitQueue-netQueues-a.patch and http://developer.wz2100.net/attachment/ticket/3399/0005-Fix-non-critical-memleak-in-NETinitQueue-netGameQueu.patch seemed to work, are you sure the explicit .clear() methods / destructors you've added in http://developer.wz2100.net/attachment/ticket/3395/0001-Cleanup-some-leaks-in-the-NET-routines.patch are really necessary?

No, not needed per say, it is just one less thing to worry about, so when my routines go checking, they can see that memory was cleared.

Hmm, yeah, MAX_PLAYERS and MAX_CONNECTED_PLAYERS are currently defined as the same thing. But still, the NETinitQueue() for the game queues uses MAX_CONNECTED_PLAYERS. If someone were later going to change MAX_PLAYERS != MAX_CONNECTED_PLAYERS, there might be unnoticed surprises. What do you think, would it make sense to either have two for-loops with MAX_CONNECTED_PLAYERS and MAX_PLAYERS each in NETdeleteQueue() or otherwise simply removing one of these defines?

For this stuff, MAX_PLAYERS would make the most sense, seems we (ab)use this in other parts of the game when we are dealing with scavs / AIs...

From http://developer.wz2100.net/attachment/ticket/3395/0002-Cleanup-leak-in-effects.patch:

free(chunkList.first);

Are you sure that only deleting the head of this list is sufficient? Compare with http://developer.wz2100.net/attachment/ticket/3399/0002-Fix-non-critical-memleak-in-initEffectsSystem-chunkL.patch

It isn't only deleting the head, that just cleans up the last one left over from the free routines above it. I don't see anymore leaks from those calls.

shutdownEffectsSystem(); // Clean up old chunks

If I'm not missing something then this shouldn't be necessary due to both initEffectsSystem() and systemShutdown() already calling shutdownEffectsSystem(), right? But I agree, it looks more sane having the effects system cleanup in the stageThreeShutDown() path instead of having them in initialization code paths. What do you think, would it make sense to either remove the shutdownEffectsSystem() you added for now (and maybe refactor this later, also other parts of the code seem to have mixed ways of doing the init and freeing stuff)? Or otherwise removing the shutdownEffectsSystem() from initEffectsSystem()/systemShutdown() in your patch as well?

Perhaps. All the init/shutdown routines are screwy, it really depends how you exit the game, and / or if you are loading savegames.
Still have to go over all this stuff again, these were not meant as final, or they would have been pushed. :)

For the other things, you fixed, nice, too! Didn't have a chance to look at them or test them in detail yet though. But I'm excited to see how few errors will be left in valgrind :).

PS: Out of curiousity, how did you produce your memory leak reports? Also, how many game rounds did you start and how long did you play to produce these quite a bit larger amounts of lost bytes?

I am using custom routines that hook directly to the allocators, and with each allocation, I do a stack dump, then on cleanup, I remove that entry.

vexed commented

Cleanup leaks in floodbucket

refs #3395

• Changeset: [/changeset/3228514dad2729e919340338020d1fecd1ca98f2 3228514dad2729e919340338020d1fecd1ca98f2]
• URL: [3228514]dad2729e919340338020d1fecd1ca98f2

vexed commented

Cleanup leaks in UTF conversion of command line (windows only) refs #3395

• Changeset: [/changeset/2a7a8acc7d78029b934b0682cb8756f0b74a1747 2a7a8acc7d78029b934b0682cb8756f0b74a1747]
• URL: [2]a7a8acc7d78029b934b0682cb8756f0b74a1747

vexed commented

Move NETinit() routines to better location

refs #3395

• Changeset: [/changeset/7d5315e3ee01cf255ac5a057517ef4b905a2dbd2 7d5315e3ee01cf255ac5a057517ef4b905a2dbd2]
• URL: [7]d5315e3ee01cf255ac5a057517ef4b905a2dbd2

vexed commented

Cleanup some leaks in the NET* routines.

refs #3395

• Changeset: [/changeset/2d5f8587b938439b11f2e8f815f7c17f46ee77c4 2d5f8587b938439b11f2e8f815f7c17f46ee77c4]
• URL: [2]d5f8587b938439b11f2e8f815f7c17f46ee77c4

vexed commented

still working on the other two patches, they have some issues with campaign games and the crazy way things get swapped.

vexed commented

Cleanup leaks in floodbucket

refs #3395

• Changeset: [/changeset/3228514dad2729e919340338020d1fecd1ca98f2 3228514dad2729e919340338020d1fecd1ca98f2]
• URL: [3228514]dad2729e919340338020d1fecd1ca98f2

vexed commented

Cleanup leaks in UTF conversion of command line (windows only) refs #3395

• Changeset: [/changeset/2a7a8acc7d78029b934b0682cb8756f0b74a1747 2a7a8acc7d78029b934b0682cb8756f0b74a1747]
• URL: [2]a7a8acc7d78029b934b0682cb8756f0b74a1747

vexed commented

Move NETinit() routines to better location

refs #3395

• Changeset: [/changeset/7d5315e3ee01cf255ac5a057517ef4b905a2dbd2 7d5315e3ee01cf255ac5a057517ef4b905a2dbd2]
• URL: [7]d5315e3ee01cf255ac5a057517ef4b905a2dbd2

vexed commented

Cleanup some leaks in the NET* routines.

refs #3395

• Changeset: [/changeset/2d5f8587b938439b11f2e8f815f7c17f46ee77c4 2d5f8587b938439b11f2e8f815f7c17f46ee77c4]
• URL: [2]d5f8587b938439b11f2e8f815f7c17f46ee77c4

vexed commented

Cleanup leaks in floodbucket

refs #3395

• Changeset: [/changeset/3228514dad2729e919340338020d1fecd1ca98f2 3228514dad2729e919340338020d1fecd1ca98f2]
• URL: [3228514]dad2729e919340338020d1fecd1ca98f2

vexed commented

Cleanup leaks in UTF conversion of command line (windows only) refs #3395

• Changeset: [/changeset/2a7a8acc7d78029b934b0682cb8756f0b74a1747 2a7a8acc7d78029b934b0682cb8756f0b74a1747]
• URL: [2]a7a8acc7d78029b934b0682cb8756f0b74a1747

vexed commented

Move NETinit() routines to better location

refs #3395

• Changeset: [/changeset/7d5315e3ee01cf255ac5a057517ef4b905a2dbd2 7d5315e3ee01cf255ac5a057517ef4b905a2dbd2]
• URL: [7]d5315e3ee01cf255ac5a057517ef4b905a2dbd2

vexed commented

Cleanup some leaks in the NET* routines.

refs #3395

• Changeset: [/changeset/2d5f8587b938439b11f2e8f815f7c17f46ee77c4 2d5f8587b938439b11f2e8f815f7c17f46ee77c4]
• URL: [2]d5f8587b938439b11f2e8f815f7c17f46ee77c4

cybersphinx commented

Replying to Warzone2100/old-trac-import#3395 (comment:20):

Cleanup some leaks in the NET* routines.

refs #3395

• Changeset: [/changeset/2d5f8587b938439b11f2e8f815f7c17f46ee77c4 2d5f8587b938439b11f2e8f815f7c17f46ee77c4]
• URL: [2]d5f8587b938439b11f2e8f815f7c17f46ee77c4

Causes #3447.

T_X uploaded file 0001-Fix-memleak-in-initEffectsSystem-chunkList.patch (2.3 KiB)

T_X uploaded file 0001-Fix-non-critical-memleak-in-createHeader-dbgHeader.patch (2.5 KiB)

T_X uploaded file 0002-Fix-memleak-in-initEffectsSystem-chunkList.patch (2.3 KiB)

T_X uploaded file 0003-Fix-memleak-in-mapLoad-tileset.patch (1.9 KiB)

T_X uploaded file 0004-Fix-memleak-in-mapLoadGroundTypes-psGroundTypes-i-.t.patch (2.7 KiB)

T_X uploaded file 0006-Rename-allocateName-to-getNameIDRef.patch (3.9 KiB)

T_X uploaded file 0007-Fix-memleak-in-loadViewData-psViewData-pName.patch (3.1 KiB)

T_X uploaded file 0008-Fix-memleak-in-loadBodyStats-psStats-pName.patch (2.2 KiB)

T_X commented

Please ignore '0001-Fix-memleak-in-initEffectsSystem-chunkList.patch', it's identifcal to '0002-Fix-memleak-in-initEffectsSystem-chunkList.patch'.

Replying to [comment:7 vexed]:

It isn't only deleting the head, that just cleans up the last one left over from the free routines above it. I don't see anymore leaks from those calls.

Hm, I just checked: Reduced the effect chunk size to 10, verified that some extra effect chunks got allocated and quit the game. And then I still had items in the chunkList at the end of shutdownEffectsSystem(). (and if I'm not missing something, then I think the above routines only clean up those elements of type 'EFFECT', not the 'struct EffectChunk' ones - a little confusing maybe :) )

I also don't see yet why you've added this extra call to shutdownEffectsSystem() in your patch. It's called fine from systemShutdown() already, isn't it?

(therefore I've added 0002-Fix-memleak-in-initEffectsSystem-chunkList.patch again, the same one as in #3399)

What do you think about 0007 and 0008 in particular, could that be a shorter alternative to 0006-Cleanup-some-viewdata-leaks.patch?

T_X uploaded file 0005-Fix-memleaks-caused-by-allocateName-calls.patch (7.4 KiB)

T_X changed _comment0 which not transferred by tractive

T_X commented

I'm also thinking about removing some "pName = strdup()" assignments by getting the according strings into the balanced tree, too. Would something like

awk 'BEGIN { FS = "," } ; { print $1" \"*"$1"*\"" }' data/base/messages/*.txt | sort | uniq

cover all those remaining strings? What's the recommended way to get this list into the tree then? Then I could also make the according pNames a (const char*) instead of (char*) and could remove the to (char*) introduced in patch 0005 in allocateName().

Or alternatively, instead of adding .txt files, changing allocateName() to do what it might have originally been supposed to do: Only allocating a new string if it's not resourced yet and if newly allocated, inserting it into the tree, too.

Hmm, this string tree is kind of helpful and very easy to use. And such a changed allocateName() could be used in a lot more places through out the code then. But on second thought this might encourage over abusing this string tree (like just throwing any string in there) - and if abused, this tree might hide memory leaks from tools like valgrind in the future. And there are STL or Qt map containers basically doing the same as this custom balanced tree implementation anyway... And probably more RAII patterns should be used, too...

Hrm, dunno. Let me know what you think about it and what you'd prefer.

T_X changed _comment1 which not transferred by tractive

T_X changed _comment2 which not transferred by tractive

vexed commented

Cleanup leaks in floodbucket

refs #3395

• Changeset: [/changeset/3228514dad2729e919340338020d1fecd1ca98f2 3228514dad2729e919340338020d1fecd1ca98f2]
• URL: [3228514]dad2729e919340338020d1fecd1ca98f2

vexed commented

Cleanup leaks in UTF conversion of command line (windows only) refs #3395

• Changeset: [/changeset/2a7a8acc7d78029b934b0682cb8756f0b74a1747 2a7a8acc7d78029b934b0682cb8756f0b74a1747]
• URL: [2]a7a8acc7d78029b934b0682cb8756f0b74a1747

vexed commented

Move NETinit() routines to better location

refs #3395

• Changeset: [/changeset/7d5315e3ee01cf255ac5a057517ef4b905a2dbd2 7d5315e3ee01cf255ac5a057517ef4b905a2dbd2]
• URL: [7]d5315e3ee01cf255ac5a057517ef4b905a2dbd2

vexed commented

Cleanup some leaks in the NET* routines.

refs #3395

• Changeset: [/changeset/2d5f8587b938439b11f2e8f815f7c17f46ee77c4 2d5f8587b938439b11f2e8f815f7c17f46ee77c4]
• URL: [2]d5f8587b938439b11f2e8f815f7c17f46ee77c4

Per uploaded file valgrind-edited.txt (820.2 KiB)

Attaching a recent valgrind report from letting the game play for a while with several different AIs, then saving, then loading, then quitting.

Cyp changed status from new to closed

Cyp changed resolution from `` to worksforme

Cyp commented

Not currently seeing any relevant remaining valgrind warnings now.