Memory leak in Surface and TileMap

[LordEidi]

The memory handling when dealing with surface is asking for troubles and is in my case resulting in memory leaks which crashes the application.

There are some ifs where memory is allocated with new. But never released:

ret->palette = new Pen[256];

ret->data = new uint8_t[needed_size];

The problem is, that palette as well as data can not be freed in a (missing) Destructor. Since it is not clear if memory was allocated with a new or not when destructing the struct. In our own copy of the 32blit code and for testing reasons we introduced variables we set when a new is used which we then check in the Destructor. That solves the memory leak. But on a maintainability and robustness perspective this is only making things worse than better.

Problem is also, that good practice would mean to use shared_ptr and similar instead of raw pointers. But since surface is used in multiple ways, this will probably mean a rewrite of the whole surface.

[ahnlak]

I think I prefer your current approach of flagging when memory has been allocated (and thus needs tidying up) rather than getting into stuff like shared_ptrs, but that might be a side effect of having learned C++ >10 year ago :smile: - I'd also want to look at performance for what is going to be a heavily used pointer.

Maintainability shouldn't be an issue, because anyone creating code that allocated memory should be thinking about where it gets deallocated anyway.

[LordEidi]

I would highly recommend to only use shared_ptr and such. Especially in such critical places. You are somewhat forced to think about who owns a pointer and who takes care of it and its destruction. Or at least it is clear depending on the kind of pointer you choose. Something which other languages like Rust depend heavily on.

And regarding performance. If you do this:

blit::set_screen_mode(blit::ScreenMode::hires);

you implicitly create two surfaces. One of which is thrown away immediately. I think not using shared_ptr because of performance is the least of your problems there...

[LordEidi]

And please, nobody take this criticism personally. I try to find the accepted way to fix this before investing too much time into the wrong solution.

[ahnlak]

It's a philosophy, for sure - I would personally prefer Keeping It Simple (mostly because I am), and would worry that bringing in "fancy C++" just because it's (this years) best practice might make the code less approachable to more people. It would certainly feel like a wholesale adoption of shared_ptr would be a sledgehammer to crack a small leaky nut.

But as I said, I'm also aware that this is just a philosophy too, and there's nothing to say which is right :smile: Tis the joy of open source.

On the set_screen_mode thing I'm not sure I quite see where the second one is (the underlying API call just hands back the appropriate framebuffer?) but there may be a quirk of C++ that needs more coffee to spot - however, that's probably a function that is not called every frame, so I imagine not much energy has been put into optimisation there.

I only mentioned the performance side because obviously Surfaces and their internal pointers are used heavily every single frame, and while the blit is a beast I'm wary of eating clock cycles at that level if we don't need to. But I've rarely (if ever?) used a shared_ptr so I have no hard ideas one way or the other what the implications are, beyond my automatic assumption that there must be some overhead for doing something where before was nothing.

[LordEidi]

Regarding set_screen_mode:

void set_screen_mode(ScreenMode new_mode) { auto &new_screen = api.set_screen_mode(new_mode); screen = Surface(new_screen.data, new_screen.format, new_screen.bounds); screen.palette = new_screen.palette; }

that auto variable is a surface you get from the API. The code then creates another surface with the same parameters of the surface just created. And adds a pointer to a palette. Coming from the same first surface.

Who owns that pointer? The new_screen, or the newly created surface? Who will take care of removing it after use? new_screen can't, since it will be removed after the scope of the set_screen_mode function ended.

Regarding speed and performance I found this older article:

https://www.modernescpp.com/index.php/memory-and-performance-overhead-of-smart-pointer

Does not seem to make much of a difference in an optimised build. But will come with the added effect of pointers getting cleaned up when there are no more objects pointing to it.

And regarding fancy. Smart Pointers are exactly what I would call making modern c++ more approachable by inexperienced users. Since the memory handling is done for them by the compiler. The language feels more like a high-level programming language where memory management is taken care of. And in that special case regarding fixing pointers within the surface, normal users of the 32blit framework will not even notice and see. Except their application not crashing when loading new resources when changing levels...

[ahnlak]

new_screen is just a reference to the internal framebuffer; it's not a whole new Surface.

However, this is probably getting wildly off topic; my personal opinion is a preference for the simple life, but I'm sure others will have alternate opinions and it can be up to @Gadgetoid to pick which PR best fits :rofl:

[Daft-Freak]

In the case of the screen data (and I think palette) point to a static buffer. So nothing really owns it.

There are four things data can point to:

• The framebuffer (static)
• const data from load_read_only
• The data pointer passed to load (usually a static buffer)
• Allocated by load_from_*

I guess it ended up this way because there are more paths that require not deleting data than do. (I think the only time palette isn't allocated is the screen though).

I think someone mentioned using a "smart" pointer for this before. Don't think changing anything is possible without breaking someone's code (don't think I'm the only one with delete[] surf->data; delete surf; in various places...) Though, can't really say anything about breakages when I'm trying to break the TileMap format myself...

(At one point I suggested at least adding an ownership flag and I think the response was that I was more likely to cause extra breakage :shrug: )

[ahnlak]

To be honest I rather like the ownership flag idea, in that it's in keeping with the "light touch" API approach of letting people do it whatever way they feel comfortable with, but defaulting to doing the right(ish) thing.

As for breakages; I would imagine anyone already deleting that data would understand why changes were necessary - as it stands, you're deleting memory from user code that was never allocated in user code, which is icky.

[LordEidi]

See Pull Request #655

[Daft-Freak]

Something I just realised: since api.set_screen_mode returns a reference to a Surface, changing the data/palette pointers to shared_ptr would be a big enough API break to break even the updater... (set_screen_mode being the one unavoidable "API" function)