Various minor issues (should be fixed) when running with Valgrind

[movax20h]

A bunch of conditional jumps on uninitialised variables and "minor memory" leaks. Not sure if memory leaks ones doesn't look too serious, but the conditional jumps should probably be fixed.

One of the issues in deflate. I tried checking the zlib 1.2.9 - 1.2.11, but the line numbers don't match exactly, suggesting you did a bit of modifications to it. I did run zlib self tests with valgrind, and it reports no issues. But they are rather primitive tests. Also, I discovered https://github.com/ebiggers/libdeflate , with a bit of vector instructions use to speed things up, could improve saving speed in Factorio, but really I think using LZ4 or zstd is probably a better idea for the future (after some benchmarks of course).

The memory leaks only show after exiting the game. During game play valgrind reports on memory leaks, so there is no on-going leaks really.

For example preloaded globally sprites aren't really an issue, even if it shows 120MB of "leaks".

Check the attachment for details.

[Rseding91]

Not surprisingly: the leaks come from the 3rd party C libraries we use. If they aren't happening runtime I don't really think it's worth looking into those.

The deflate one has a comment right on the line where it's mentioning conditional jump or move depends on uninitialized value(s):

/* If n is not on any hash chain, prev[n] is garbage but
* its value will never be used.
*/

Which makes me think it's also a non-issue and just another poorly written C library. But, I could be wrong. that's just a quick glance at the lines it mentions.

[movax20h]

I would definitively ignore the deflate one for a moment. I think it is a real bug, but I will report it to deflate upstream mainling list probably instead. I should be able to reproduce it easily on my own.

The other ones are conditional jumps on uninitalized value in update buffer renderer (UniformBuffer.hpp:47) were happening doing my short play in introduction campaign and were happening when I mined a rock or opened a chest with existing items. I got them reliability in multiple tests.

I will retry with valgrind origin tracking but the framerate will probably drop from 0.3 fps to 0.1 fps

[posila]

We have not updated zlib for a long time (apparently we use version 1.1.4, hmmm)

I think I fixed the UniformBuffer thing for next release. The class caches content of the uniform buffer, so that when update is called on it, it doesn't actually update it on GPU if the content is same (comparing new content with cache using memcmp). Because size of uniform buffer has to be multiple of 16 bytes, some of the uniform buffer POD structures have dummy values for padding, that are unused and uninitialized, which is what I think tripped valgrind. So we are zero-initializing those structures now. It was not a critical bug, only possibly inefficient because it would update UB on GPU when it was not actually needed.

I am not sure what's up with the leak in libpng, once the game starts to menu, there shouldn't be any PNGs in memory anymore, but I don't want to look into it now.

[movax20h]

Fair enough. Thanks.

[movax20h]

I think I fixed the UniformBuffer thing for next release. The class caches content of the uniform buffer, so that when update is called on it, it doesn't actually update it on GPU if the content is same (comparing new content with cache using memcmp). Because size of uniform buffer has to be multiple of 16 bytes, some of the uniform buffer POD structures have dummy values for padding, that are unused and uninitialized, which is what I think tripped valgrind. So we are zero-initializing those structures now. It was not a critical bug, only possibly inefficient because it would update UB on GPU when it was not actually needed.

I can confirm that the UniformBuffer uninitialized variable thingy is now fixed.

I did discover some small memory leaks related to ChangelogGui in 0.17.79, would you like details here or on a separate issue?

It is mostly coming from ChangelogGui::showVersion(Version const&, Version, bool) ; ChangelogGui.cpp lines 223 and 224 that is called from onItemSelect. Looks like ScrollBar action listener is not freed (line 224), and scroll pane prive child widget not being freed either (line 223). It also looks like TextBox action listener is not removed either. It also looks that there is a naked new call on line 224, and it is never removed of its owner ship transferred anywhere.

[moon69]

Just curious really...
If someone looks into the zlib stuff at some stage, perhaps you could look at why 'autosave-compression-level=none' is still setting the compression method to Deflate (8) rather than Store (0)?

The compression level appears to be "no compression", so it's fast, but Deflate does appear to introduce some extra overhead - whole kb's of my SSD is getting swallowed with every save

I first posed the question here...
Zip file compression metadata curiosity

[Rseding91]

I fixed it for the next release so it will mark them as stored when no-compression is used.