News:

Simutrans.com Portal
Our Simutrans site. You can find everything about Simutrans from here.

[Bug] Memory leak

Started by Mariculous, July 17, 2019, 11:43:40 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Mariculous

Hey there,
I was observing some unexpectedly high memory usage of my simutrans-extended server. While running, memory usage will significantly increase.
When starting the server, it uses ~720mb, increasing while it is running. In a test run for a few days it consumed nearly 32 GB, then I canceled the run by restarting the server and it was back at ~720mb.

I know this is not pretty useful but I do also know that I don't know which exact informations you need and how do get these.
Trying out some stuff like reconnecting to the server many times, artificially growing cities, replacing vehicles and more, I did not find out any cause yet.

DrSuperGood

Providing the save and listing the pakset used would be a useful start.
Quote from: Freahk on July 17, 2019, 11:43:40 AMTrying out some stuff like reconnecting to the server many times, artificially growing cities, replacing vehicles and more, I did not find out any cause yet.
So those activities did not result in a memory leak?

Mariculous

#2
I am using pak128.britain.
Savegame can be found here: https://dome.xileks.de/simutrans/save/2019071714.sve

I can't tell this exactly for any of these but it seems these activities don't.

Reconnects forcing a map reload and it seems some data is thrown away at reaload so memory usage drops by a few mb. However, sometimes when some ppl are reconnecting directly after each other, the memory usage increases by very few mb but this diesn't seem to be the issue.

Increasing cities itself didn't add any memory consumption at all, however a few minutes after increasing some cities by a total of 6000 inhabitants, memory consumption increased by ~100 mb.
However, even if that consumption was a leak caused by the growth, with the current city growth on my server of a total ~8000per year, it would take round about 31GB/120 mb/ingame year ~= 258 ingame years, which is much longer than the server was online so I don't think that's the cause but I will setup a test server without restarts and city growth disabled on all cities to confirm this.

Replacing vehicles takes much time on my routes because of the low vehicles speed (12 km/h when I did the test) and long distances.
While replacing, memory consumption didn't seem to increase more or les than at times where I did not replace vehicles but I will have alook at it again in future years when faster vehicles are available.

jamespetts

As Dr. Supergood says, I will need a reproduction case to look into this. I should note that I am very busy this week and next, so it may be a while before I am able to look into this. Any testing to narrow the issue in the meantime would be appreciated.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Mariculous

Oh you replied to this in the meantime.
I have updated my previous post with the savegame.
The server is running #dc58b30 of 8th July but according to git there were no changes after this date yet.

Be sure I will try to reproduce this on my local machine but this could take a few days.

DrSuperGood

Might have something to do with end of months. There is a reason why in large maps there is a freeze at the end of the month.

Might also have something to do with way reservations. Maybe there is a leak in some edge case which piles up over time.

Might have something to do with the path explorer. Every time the path explorer completes a full cycle maybe there is some memory leaked. Hence over time memory leaks accumulate.

Ranran(retired)

I do not know if it is related to this problem, but as I previously reported here, memory usage increases each time you click in the depot window. (Even if you don't buy a vehicle)
It does not occur in standard.
ひめしという日本人が開発者達の助言を無視して自分好みの機能をextendedに"強引に"実装し、
コードをぐちゃぐちゃにしてメンテナンスを困難にし(とりわけ道路と建物関連)、
挙句にバグを大量に埋め込み、それを知らんぷりして放置し(隠居するなどと言って)別のところに逃げ隠れて自分のフォーク(OTRP)は開発を続けている
その事実と彼の無責任さに日本人プレイヤーは目を向けるべき。らんらんはそれでやる気をなくした(´・ω・`)
他人の振り見て我が振り直せ。ひめしのようにならないために、らんらんが生み出したバグや問題は自分で修正しなくちゃね(´・ω・`)

Mariculous

#7
The memory consumption does not seem to be created by any user interaction. I startet a server on my local machine and am monitoring its mem usage. I did not connect to it and memory usage seemed to be constant until some point when it suddenly jumped from ~4.1% to 4.2% at the next month change it jumpedagain to ~4.6%

I guess DrSuperGood could be correct and it leaks memory about the month change but can't say this for sure before some more months have passed.

Edit: after connecting to the server it dropped a bit, still consuming more mem than before the month change.
See the graph for details.

DrSuperGood

Quote from: Freahk on July 17, 2019, 04:25:20 PMThe memory consumption does not seem to be created by any user interaction. I startet a server on my local machine and am monitoring its mem usage. I did not connect to it and memory usage seemed to be constant until some point when it suddenly jumped from ~4.1% to 4.2% at the next month change it jumpedagain to ~4.6%
Can you prepare a save of such a game just before a month ends? Specifically only a few seconds away. If there is a memory leak when the month changes a developer should be able to spot what is leaking via instrumentation. One can then work on resolving why the leak is happening.

When I spotted a major memory leak last year I only tested between save/load cycles. I did not test between game months.
Quote from: Ranran on July 17, 2019, 02:10:41 PMI do not know if it is related to this problem, but as I previously reported here, memory usage increases each time you click in the depot window. (Even if you don't buy a vehicle)It does not occur in standard.
This should not be affecting graphicless servers as they have no UI and potentially minimal human interaction.

Mariculous

#9
Sure I can, will provide a savegame soon.
I just tested the issue in singleplayer using ff now. It seems that the memory usage does not only jump at month change but round about 6 times a month.

I will prepare a savegame just before a month change as I don't know exactly when the other memory usage jumps do happen.

Edit: It seems that memory jumping up at other times than month changes only appears in ff appears much more often in ff than at normal speed.
I won't do any speculations here since I don't know the code.

There it is: https://dome.xileks.de/simutrans/save/1823_12_6:23:00.sve
Just one minute before the month change after which the game consumed ~150mb mre than before the change

DrSuperGood

#10
The memory usage spike appears related to the following two entries...

Simutrans-Extended Normal x64 Debug (SDL 2).exe!route_t::ANode[] +6 +288,001,464 +6 12 576,002,928
Simutrans-Extended Normal x64 Debug (SDL 2).exe!quickstone_hashtable_tpl<haltestelle_t,haltestelle_t::connexion *>[] +5,215 +12,954,060 +5,215 89,134 221,408,856

These are the path explorer. The memory did not decrease after the path explorer finished running. The initial spike of ~300MB is not a leak. Rather it is the path finder caching some ANode[] for internal use which are recycled between runs.

Possibly these ANode[] should be created on map load as they require significant memory. Either that or they should be freed between path explorer runs.

There is possibly a leak with the following...

Simutrans-Extended Normal x64 Debug (SDL 2).exe!quickstone_hashtable_tpl<haltestelle_t,haltestelle_t::connexion *>[] +11,473 +28,498,932 +11,473 98,604 244,932,336


Between path explorer first starting and finishing there was a gain of nearly 30MB of quickstone_hashtable_tpl. Like wise there was a gain of nearly 40MB of them between path explorer runs. At this rate over the course of a day or so this could add up to several hundred megabytes very easily.

The sight that is creating them appears to be path_explorer_t::compartment_t::step...
Identifier Count Size (Bytes) Count Diff. Size Diff (Bytes) Module
+ path_explorer_t::compartment_t::step 21,079 78,972,524 +16,886 +67,817,986 Simutrans-Extended Normal x64 Debug (SDL 2).exe
+ path_explorer_t::compartment_t::step 36,724 117,834,704 +15,645 +38,862,180 Simutrans-Extended Normal x64 Debug (SDL 2).exe


Keep in mind that no modifications are being made to routes in this save game. After the first path explorer run any subsequent should result in very similar state, maybe with slightly different timings.

This is the line that appears to be generating them...

connexion_list[ all_halts_list[i].get_id() ].connexion_table = new quickstone_hashtable_tpl<haltestelle_t, haltestelle_t::connexion*>();

Since connexion_list is a static array maybe it is making new hashtable objects without ever deleting them?

Also noticed that that an array bounds overflow might be possible if more than 65536 stops exist?

I can confirm this is a memory leak. Replacing the above with the blow completely removes the leak. Between path explorer runs a total delta of just 2MB occurs (most of which was transferring cargo) as opposed to the 40MB before. To put it in perspective this change makes the total allocation delta from load to the end of 2 path explorer runs go from +37,000 allocations to just +4,000 allocations with a reduction of memory used of around 70MB.

if (connexion_list[ all_halts_list[i].get_id() ].connexion_table == nullptr) {
connexion_list[ all_halts_list[i].get_id() ].connexion_table = new quickstone_hashtable_tpl<haltestelle_t, haltestelle_t::connexion*>();
}

This is quite a significant leak and the leak scales with the number of stops built. This easily explains why the Bridgewater Brunel server is performing like mud and crashing after a few in game months as that has a lot more stops so the leak is likely closer to 100-200MB or so per path explorer cycle.

ACarlotti

#11
I introduced the memory leak in my commit on May 12; for some reason I thought that the connexion_tables wouldn't be initialised at that point. I'm about to test a fix, which is essentially to delete that line so that connexion_table's are only initialised when the path explorer is initialised and when halts are initialised.
The change I made on June 21 would have had no effect on the memory leak, but should have reduced the total memory usage regardless. Unfortunately a constant reduction in memory usage doesn't compensate for the huge linear memory leak.

EDIT: Now on Github. I tested it in Valgrind through a couple of path explorer cycles, and there are no longer leaked allocations of connexion_tables in halt initialisation or the path explorer step (both leaked through the same bug due to the swapping mechanism used in the path explorer).

I'll try to ensure I test any similar changes in Valgrind in future.

This fix, along with the improvement I wrote last month, should mean that the Bridgewater-Brunel server can run using less than 8GB of RAM.

EDIT 2: I think it currently needs around 7GB of RAM, which means that James should be able to run it on his computer again. I think there is still scope for another 1GB of memory savings (by reducing the precision of the precomputed journey times generated by the path explorer).

DrSuperGood

Quote from: ACarlotti on July 24, 2019, 09:45:43 PMThis fix, along with the improvement I wrote last month, should mean that the Bridgewater-Brunel server can run using less than 8GB of RAM.
The server should become a lot more stable and performant as much less page trashing should occur after a few path explorer cycles. Shortly before it crashes at the moment it can take upwards of 30 minutes to reload a save and advances state at a rate of seconds per frame.
Quote from: ACarlotti on July 24, 2019, 09:45:43 PMEDIT 2: I think it currently needs around 7GB of RAM, which means that James should be able to run it on his computer again.
This should not really be an issue for much longer. James will be getting a pretty high end system with 32GB of memory soon.

ACarlotti

Quote from: DrSuperGood on July 18, 2019, 02:25:16 PMAlso noticed that that an array bounds overflow might be possible if more than 65536 stops exist?
I forgot to mention before - there is a hardcoded limit of 65535 stops, so this will never be an issue. The limit exists because halt ids are stored as uint16s; increasing this would have a significant effect on memory usage for smaller games, without changing the fact that games with more than 65535 stops would be too computationally expensive.

DrSuperGood

Quote from: ACarlotti on July 25, 2019, 11:06:37 AMI forgot to mention before - there is a hardcoded limit of 65535 stops, so this will never be an issue. The limit exists because halt ids are stored as uint16s; increasing this would have a significant effect on memory usage for smaller games, without changing the fact that games with more than 65535 stops would be too computationally expensive.
This value should be made and used as a constant rather than hard coded magic number. For example STOP_ID_MAX. That way it is clear the limit is consistent and likely declared by technical reasons rather than some arbitrary magic number which I originally thought it was.

Some memory could be saved by dynamically resizing the array as required up to STOP_ID_MAX. Not sure if it is worth it or not as likely will be a couple of kilobytes maximum and may have some performance implications. For example the server game, which is likely one of the most complex Extended will see in a long time, is at only 8,413 stops so under a quarter of the array is being used.

ACarlotti

Actually, I think this array is probably using around 150MB at present (due to hashtables being rather inefficient). The hashtables should be improved first though, and that should remove any further need to reduce memory usage here.

jamespetts

Thank you very much to A. Carlotti for the fix for this and to Freahk for reporting it: that is much appreciated. I have now incorporated this fix.

My apologies for not having the chance to look into this until now: I have, as indicated above, been very busy at work in the last few weeks. Also, I am still awaiting the availability of essential components to build the new computer that I need to replace my failing old computer that I am currently using, which makes using the computer unnecessarily difficult (owing to low speed and lack of memory), so I am prioritising incorporating fixes/improvements by others at present.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.