Skip to main content ↑ ↓
Topic: [r2826] Tiny bug and other potential problems (Read 2957 times) previous topic - next topic

[r2826] Tiny bug and other potential problems

Just out of curiosity I ran simutrans through Valgrind and discovered a tiny bug. The patch speaks for itself. It's not a very serious bug, but it's better to be safe than sorry. Hopefully the patch is correct.

I also got a lot of messages from Valgrind about invalid reads that made no sense to me. Some in factory_reader_t::register_obj, some related to calls to haltestelle_t::get_halt and the rest scattered about. Have anyone else encountered these?

Valgrind also reported a lot of memory leaks. Some indicate that SDL was not properly shut down or bugs in SDL, while others may indicate that an instance of karte_t is not properly disposed of. A search on this forum show that reported memory leaks apparently are a well known "problem", though.

I can post the full log if someone is interested.

Re: [r2826] Tiny bug and other potential problems

Reply #1 –
Quote from: Ters – on
A search on this forum show that reported memory leaks apparently are a well known "problem", though.

I must admit, that I used to tolerate a number of memory leaks. A few kb that get lost each time a map is created or loaded, or on other infrequent operations didn't look so troublesome to me - also for many years I had no tools to detect such at all.

I'm fairly sure, that most versions didn't continuously eat memory, what I'd consider a real "leak". I hope the late, stable releases also behave this way, and thus the leaks should not have too bad effects.

But yes, it could be better, but is hard to clean up now.

Re: [r2826] Tiny bug and other potential problems

Reply #2 –
Most leaks are due to the problem, that I use a self made memory mamangement which is five times faster and has less overheads compared to (old) vanilla malloc. To free this memory, its destructor needs to be called after all other destructors have been destructed, which is not feasible in a portable was, as far as I know.

The same is true for the last karte, it needs to be destroyed after all other objects have been destroyed and the display is turned off. It is possible, but for destruction is is much faster to just let the OS remove all memory. I never saw ever an OS that left memory of an application allocated after termination (apart from the one marked for exactly that purpose).

Many of the actual errors are also within SDL and thus not really solvable for simutrans.

Re: [r2826] Tiny bug and other potential problems

Reply #3 –
Are all objects allocated using new routed through this memory management system? I don't see anything indicating that they are. This seems to be done on a cl**** by cl**** basis. Since most of the leaks are objects belonging to a cl**** without an overridden new operator, I do not think the leak reports are caused by the self made memory management system.

Even if many of the reported leaks are not that problematic, it is harder to spot the real problems when they are hidden among lesser issues. I have attached a patch that silences many leak reports. Hopefully they have no negative side effects.

I might use the remaining leak reports as a learning exercise in both Valgrind and the simutrans source code. Apart from the SDL related leak reports, Valgrind lists many leaks related to karte_t's plan array.

Re: [r2826] Tiny bug and other potential problems

Reply #4 –
I am a little ambivalent of this. I could add them: But they take longer to exit the program and often could lead to crashes due to unexpected initialisation order of the destructor (or they rather need a correct destructor). But to free some of those might be even needed to get the memory from the hard disk (if it was an unused menu for instance) just to free it. The OS is usually much faster at freeing memory, since it does not care about its previous content.

And due to the way the paks are loaded and most objects are initialized (like the most frequent ones, trees and houses) those are very hard to properly reclaim the memory. Those not be sucessful freeed without some major efforts (which I find not worth for the arguments given above.)

What I am concerned are real leaks.

Re: [r2826] Tiny bug and other potential problems

Reply #5 –
Those are valid concerns. I just needed to see if I was correct in guessing where the problem was so that I could rule them out. Posting a patch was then just a small effort.
 

Re: [r2826] Tiny bug and other potential problems

Reply #6 –
Probably I will add them, but they will only called debug level>3. That way it will be useful to find errors and yet not hinder normal games.