Reconsider / Remove MemoryManager

[TechnoPorg]

Motivation / History

Most of LMMS' memory management code has not been touched in quite a number of years, and both C++ and computer hardware itself have changed enough since then that I think it needs a thorough second look.
I'm going to focus on the MemoryManager class in this discussion, and leave memory pools, etc. aside.

Here's the original mailing list discussion of the MemoryManager, back when it was implemented by Vesa in #1088. It pre-allocated a big chunk of memory on application launch, which was then given to classes initialized with the MM_OPERATORS macro. #3873 replaced that poorly performant custom implementation with rpmalloc, and completely removed the pre-allocation.

Since that time, standard library implementations of allocators have become better optimized, and hardware has moved to 64-bit systems with more available memory, which makes fragmentation far less of a concern than it used to be.

lukas-w has an open PR to refactor memory management (#4443), which is in need of a rebase and mostly focuses on memory pools, as far as I can tell.

Alternative memory manager

I tested out mimalloc as a potential alternative, as it seems to consistently outperform other memory allocators (source). The relatively minor changes necessary to do so can be tested out from my branch: https://github.com/TechnoPorg/lmms/tree/mimalloc-memory-allocator. Note that you will need the mimalloc package installed locally (mimalloc-devel on Fedora).

Here are some basic test results I got using an admittedly unimpressive benchmark (run on Linux, target DEBUG, clang):

	auto start = std::chrono::high_resolution_clock::now();
	for(int i = 0; i < 1000000; i++)
	{
		auto myPort = new AudioPort("audio port");
		Q_UNUSED(myPort);
		delete myPort;
	}
	auto end = std::chrono::high_resolution_clock::now();
	auto duration = std::chrono::duration_cast<std::chrono::microseconds>(end - start);
	// BREAKPOINT HERE

Allocator	Time	Percentage of rpmalloc time
rpmalloc (current master)	1553749us	100%
default clang memory allocator (just remove MM_OPERATORS from AudioPort)	1516927us	97.6%
mimalloc (my branch)	1480876us	95.3%

These results varied pretty widely and I by no means did them in a controlled environment, so take them with a grain of salt. The mimalloc time seemed to vary the most, and was often equal to or occasionally even slightly underperforming the default allocator. What was consistent, though, is that rpmalloc performed the worst.

I did not test the total amount of memory used.

Suggestions

Personally, I think that getting rid of the custom memory manager operators entirely is the best course of action. Imho reliance on standard C++ behaviour is always the safest course of action, and using default memory management will simplify LMMS code in terms of readability and maintenance. It should also increase performance slightly over rpmalloc.

We could also investigate using mimalloc as a drop-in replacement for rpmalloc, but unless someone else can conclusively show that it's markedly better, I would say an extra dependency and the responsibility of maintaining our own memory manager are not worth a very inconsistent 2.3% improvement.

[Veratil]

I also tried removing the rpmalloc calls: https://github.com/Veratil/lmms/tree/remove-rpmalloc

My general usage test didn't run into any issues, so as a first step stripping out the rpmalloc calls should be fine at least.

[TechnoPorg]

Your branch is completely in line with what I was thinking. I was a little concerned about removing the thread guard, but seeing that you've done so makes me feel better.
Once we've stripped out the rpmalloc calls, though, would there be any point to keeping MemoryManager around? Afaik a new wrapper that just uses std::alloc doesn't actually do anything differently from the built-in new.

[Veratil]

I don't think there would be a reason to keep the manager around. My branch was just the fastest way to test removing it rpmalloc from mostly everything.

[TechnoPorg]

Great, we're on the same page then. If you like, I can open a PR now based off of your branch to remove rpmalloc/MemoryManager entirely.

(Thanks for the speedy reply btw! Much appreciated)

[jsmtux]

This would be a great change. In my branch I couldn't use the memory manager in most cases, since Qt6 does not allow the usage of placement new with QObject derived classes
https://doc.qt.io/qt-6/qmetatype.html#construct-1
I was quite unsure what to do about this, so I feel very happy that you're already thinking about removing it.

[TechnoPorg]

This has actually already been done in PR #7128! I'm glad that we're on the same page about this and it's helpful to your efforts.

[TechnoPorg]

I did some more rigorous profiling by measuring how long it takes LMMS to render this project:

Profiling commands

I used this command on both the binary built with rpmalloc, and the binary just using std::alloc:
hyperfine --warmup 3 './lmms render stanley.mmpz'
And this command on the std::alloc version, which overrode the default memory allocator so that mimalloc was being used for all memory allocations performed by LMMS, not just those performed by MemoryManager:
hyperfine --warmup 3 'LD_PRELOAD=/usr/lib64/libmimalloc.so MIMALLOC_VERBOSE=1 ./lmms render stanley.mmpz'

Results

rpmalloc:

Time (mean ± σ): 73.523 s ± 0.660 s [User: 361.299 s, System: 93.270 s]
Range (min … max): 72.501 s … 74.751 s 10 runs

Just std::alloc:

Time (mean ± σ): 73.102 s ± 0.732 s [User: 363.620 s, System: 89.925 s]
Range (min … max): 72.125 s … 74.325 s 10 runs

mimalloc:

Time (mean ± σ): 72.816 s ± 0.654 s [User: 364.976 s, System: 90.630 s]
Range (min … max): 71.635 s … 74.021 s 10 runs

---

Given that they're all within a standard deviation of each other, and it's quite a tight spread of results, I think this confirms the hunch that in the real world, the memory allocator being used has a negligible impact on performance.

[sakertooth]

Closing as resolved given that MemoryManager was removed in #7128.