Closed wzdev-ci closed 8 years ago
_TX uploaded file valgrind-suppresions
(2.1 KiB)
Valgrind suppresions file to exclude external libraries
_TX 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
_TX 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: https://github.com/Warzone2100/warzone2100/pull/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: https://github.com/Warzone2100/warzone2100/pull/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
_TX 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
vexed commented
Cleanup leaks in UTF conversion of command line (windows only) refs #3395
vexed commented
Move NETinit() routines to better location
refs #3395
vexed commented
Cleanup some leaks in the NET* routines.
refs #3395
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
vexed commented
Cleanup leaks in UTF conversion of command line (windows only) refs #3395
vexed commented
Move NETinit() routines to better location
refs #3395
vexed commented
Cleanup some leaks in the NET* routines.
refs #3395
vexed commented
Cleanup leaks in floodbucket
refs #3395
vexed commented
Cleanup leaks in UTF conversion of command line (windows only) refs #3395
vexed commented
Move NETinit() routines to better location
refs #3395
vexed commented
Cleanup some leaks in the NET* routines.
refs #3395
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.
_TX uploaded file 0001-Fix-memleak-in-initEffectsSystem-chunkList.patch
(2.3 KiB)
_TX uploaded file 0001-Fix-non-critical-memleak-in-createHeader-dbgHeader.patch
(2.5 KiB)
_TX uploaded file 0002-Fix-memleak-in-initEffectsSystem-chunkList.patch
(2.3 KiB)
_TX uploaded file 0003-Fix-memleak-in-mapLoad-tileset.patch
(1.9 KiB)
_TX uploaded file 0004-Fix-memleak-in-mapLoadGroundTypes-psGroundTypes-i-.t.patch
(2.7 KiB)
_TX uploaded file 0006-Rename-allocateName-to-getNameIDRef.patch
(3.9 KiB)
_TX uploaded file 0007-Fix-memleak-in-loadViewData-psViewData-pName.patch
(3.1 KiB)
_TX uploaded file 0008-Fix-memleak-in-loadBodyStats-psStats-pName.patch
(2.2 KiB)
_TX 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?
_TX uploaded file 0005-Fix-memleaks-caused-by-allocateName-calls.patch
(7.4 KiB)
_TX changed _comment0 which not transferred by tractive
_TX 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.
_TX changed _comment1 which not transferred by tractive
_TX changed _comment2 which not transferred by tractive
vexed commented
Cleanup leaks in floodbucket
refs #3395
vexed commented
Cleanup leaks in UTF conversion of command line (windows only) refs #3395
vexed commented
Move NETinit() routines to better location
refs #3395
vexed commented
Cleanup some leaks in the NET* routines.
refs #3395
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.
keyword_memleak
keyword_report
keyword_uninitialised
keyword_uninitialized
keyword_valgrind
resolution_worksforme
type_bug
| by T_XTest procedure:
Generated with the attached 'valgrind-suppressions' file to exclude external libraries.
Summary
Conditional jump or move depends on uninitialised value(s):
Only displaying stuff. Therefore not really harmful?
Worst Memory Leaks:
So the biggest leaks seem to be in initialization processes and should therefore not be growing during game play. Can the NetQueue::pushMessage one be exploited?
For the full valgrind.log see attachment.
So far these ones do not seem to confirm the issues reported in #3218. Still could be nice having them fixed to be able to spot regressions and more sneaky bugs in the future.
PS: Note, if you want to reuse the valgrind suppressions file, you'll probably want to add the objects from your graphics driver. I only added some for the Nvidia nouveau graphics driver.
Issue migrated from trac:3395 at 2022-04-16 09:43:32 -0700