Refactoring: Memory management #4443

lukas-w · 2018-06-21T12:04:58Z

Copying description from #4311, which for some reason GitHub won't let me reopen. I made these changes in November and April, but didn't have time to fix them on all platforms back then.

Some refactorings regarding memory management and containers.

MemoryManager.h renamed to Memory.h
MemoryHelper::alignedMalloc moved to Memory.h, ported to an stl-compatible allocator called AlignedAllocator, implementation replaced by a single rpmalloc call.
LocklessAllocator implementation replaced by a new MemoryPool class, which now also replaces the implementations of BufferPool (formerly BufferManager) and NotePlayHandlePool (formerly NotePlayHandleManager). Instead of a custom-written lockless stack, MemoryPool uses a 3rdparty lockless queue implementation which yields much better performance.
Add some tests and naïve micro-benchmarks.

This includes a a new submodule, libcds, which is a collection of concurrent and lock-free data structures.

Starting with this PR, I'd like to license my contributions as MIT, while the project as a whole is still licensed as GPL. Hope that doesn't cause any trouble.

zonkmachine · 2018-06-21T21:08:56Z

...which for some reason GitHub won't let me reopen.

Because you force pushed to the branch while it was closed?

lukas-w · 2018-06-22T14:47:29Z

That's probably it. Didn't know it would cause that.

include/Memory.h

PhysSong · 2018-06-27T08:48:14Z

benchmarks/benchmark.cpp

+
+	using Stack = LocklessList<size_t>;
+	{
+		size_t n = 100 * 1000 * 1000;


I think you should make it as constexpr.

That wouldn't make any difference, would it?

It evaluates at compile-time rather than run-time.

src/core/BufferPool.cpp

include/NiftyCounter.h

lukas-w · 2018-08-03T12:04:03Z

The merge conflicts grew a bit out of hand so I rebased on top of master another time.

src/core/NotePlayHandle.cpp

jasp00 · 2019-02-22T19:23:25Z

I'd like to license my contributions as MIT, while the project as a whole is still licensed as GPL.

I think you are wrong, but I respect your decision regarding your contributions. You can even license all your past contributions under MIT. However, there is a problem when you add MIT license headers to files because you are affecting future contributions of others. In this case, you are touching the core of LMMS; in my humble opinion, you are breaking project consensus.

Solution: state your permission in LICENSE.txt and use GPL headers for the other files.

lukas-w · 2019-02-26T08:47:11Z

I already stated my permission to re-license all my past contributions in #3412 (comment), so did a handful of other contributors.

in my humble opinion, you are breaking project consensus

I understand your concern, but I don't think this is the case here, because (a) as can be seen in discussions like #3412, there is no clear consensus on the GPL, and (b) developers who don't want to license their contributions to my files under the MIT can simply change the file's license to GPL.

jasp00 · 2019-02-26T15:09:38Z

I already stated my permission

If you do not want to change LICENSE.txt, that is fine.

developers who don't want to license their contributions to my files under the MIT can simply change the file's license to GPL.

Do you mind if I change those headers now?

lukas-w · 2019-02-26T15:39:10Z

If you do not want to change LICENSE.txt, that is fine.

LICENSE.txt is the file that contains the license the project as a whole is published under. It's not the place for this.

developers who don't want to license their contributions to my files under the MIT can simply change the file's license to GPL.

Do you mind if I change those headers now?

Yes I do, because I wouldn't call it a contribution to my files, but an attempt to enforce your opinion on others.

jasp00 · 2019-02-26T15:53:01Z

Code is authoritative. Your headers state that files are under MIT, thus contributions must be under MIT. Please be unambiguous in code, not just on GitHub issues.

lukas-w · 2019-02-26T16:04:37Z

What exactly is ambiguous in your eyes?

jasp00 · 2019-02-26T16:06:42Z

Again: your headers state that files are under MIT, thus contributions must be under MIT. Is this your intention?

lukas-w · 2019-02-26T16:20:11Z

Yes, the files are under MIT and any contributions to them will therefore be under MIT as well, unless the contributor decides to sublicense the file because he doesn't want his contributions to be under MIT.

jasp00 · 2019-02-26T16:34:13Z

unless the contributor decides to sublicense the file

This is ambiguous: sublicensing has nothing to do with contributions. You are forcing MIT onto contributors. This is a GPL project.

Hope that doesn't cause any trouble.

It does. I have told you a way to add your permission; if you do not like to use LICENSE.txt, use another file.

lukas-w · 2019-02-27T08:36:31Z

I'm not forcing the MIT onto anyone. It's compatible with the GPL and thus MIT files can be relicensed under the GPL. Please explain why you think this wouldn't be possible.

I have told you a way to add your permission

Yes but I don't wish to add permission, I wish to license the files under MIT. This makes future versions of them MIT as well as long as contributors are fine with it.

jasp00 · 2019-02-27T11:13:42Z

This makes future versions of them MIT as well as long as contributors are fine with it.

You still want to remain ambiguous, but your code and your intention are clear: you are forcing MIT in those files.

I object to your refactoring. If you go ahead, I would like a configure option to use the current implementation instead.

lukas-w · 2019-02-28T22:12:17Z

You still want to remain ambiguous

I tried to explain what I'm doing and why it's not forcing the MIT onto anyone. Licensing a file as MIT doesn't force contributions to that file to be MIT, it just makes it the default option. If I'm being unclear about something, just ask specifically and I will gladly try to clarify it. If I'm wrong about the legal implications of MIT licensing, feel free to correct me, but please at least try explaining why you think I'm wrong. I don't know what else is left to say when you insist on reiterating your statements without actually responding to what I write.

your code and your intention are clear: you are forcing MIT in those files.

Please refrain from making such claims about my intentions when I've just told you differently. I prefer not to be accused of lying or of intentionally obscuring the truth.

I would like a configure option to use the current implementation instead.

This is absurd. No such thing will happen.

jasp00 · 2019-02-28T22:43:31Z

 * This file is licensed under the MIT license. See LICENSE.MIT.txt file in the
 * project root for details.

This is what matters, not whatever you are writing in this pull request. If you want to be unambiguous, just add your explanation: "Contributions to this file are not required to be under MIT, but I request so".

tresf · 2019-03-12T16:49:56Z

I'm not forcing the MIT onto anyone. It's compatible with the GPL and thus MIT files can be relicensed under the GPL.

@jasp00 is correct. Future contributions to this file are forced under MIT unless the project removes the license.

This is fine though. We do it for CMake files. @jasp00 please let this issue rest. These files you're attacking are written by one person that's chosen to make them:

Scalable to other projects, not just LMMS
Compatible with our current license
Isolated enough from our source code that they won't cause the ambiguity that you speak of

"Contributions to this file are not required to be under MIT, but I request so".

No, they're MIT. That's where you're right @jasp00. But that's fine. If the project chooses to downgrade to GPL we can do so at any time. @lukas-w has given us the freedom to do that.

jasp00 · 2019-03-12T17:16:02Z

Compatible with our current license

Thanks. Double standard will make my life easier.

tresf · 2019-03-12T17:33:16Z

Thanks. Double standard will make my life easier.

As indicated, we already have this exception on CMake files. Assuming "double standard" means "two standards", and not the proverbial English slang for "not being treated fairly".

src/CMakeLists.txt

Resolves some inexplicable linking errors on Windows. Saves us from working around incomplete CMake support of object libraries.

lukas-w · 2020-05-22T15:27:16Z

Ok, builds should pass now. Thank you for the great help with the MSVC build @PhysSong, I applied your steps in a64f83e. Sorry for the repeated force-pushing which is something I usually try to avoid doing, but this has been a lot of trial-and-error.

JohannesLorenz · 2020-09-05T12:49:50Z

What's missing in this PR to be merged?

lukas-w · 2020-09-05T14:20:37Z

@JohannesLorenz Just review and testing I guess.

PhysSong · 2020-09-14T06:34:23Z

include/Memory.h

 };

+static MemoryManager::MmCounter _mm_counter;
+static thread_local MemoryManager::MmCounter _mm_thread_counter;


Why do you have thread local MmCounter here and ThreadGuard in Memory.cpp?

Sorry for missing this question. I don't remember exactly to be honest. The MmCounter was introduced in ac0081d to fix a crash in MSVC, IIRC it would call rpmalloc_finalize before rpmalloc_thread_finalize. I realize this whole Nifty Counter solution may be a bit fragile to use, so I'm open to searching a different solution if we can't make it work reliably.

PhysSong · 2020-12-11T03:01:00Z

@lukas-w Sorry for bumping, but could you resolve conflicts again?

lukas-w · 2020-12-11T08:50:27Z

@PhysSong Sure, done.

PhysSong · 2020-12-11T09:50:31Z

Things to check before merging:

Does the change from lmmsobjs to lmmslib make no regressions or additional issues?
The new Nifty counter logic works as expected?

Rossmaxx · 2022-06-18T11:11:56Z

Rebase?

include/AutomationTrack.h

PhysSong · 2022-07-30T12:08:03Z

Rebase?

On it.

Rossmaxx · 2024-02-26T12:32:50Z

Does this PR still has a point after #7128 ?

PhysSong · 2024-02-27T06:39:24Z

Does this PR still has a point after #7128 ?

I think so because it includes some useful changes such as memory pool, etc.

TechnoPorg · 2024-02-28T14:19:54Z

The MemoryHelper::alignedMalloc function that is changed in this PR could be replaced with std::aligned_malloc now.

As far as I can tell, the single place MemoryHelper::alignedMalloc is used is in AudioEngine, for allocating output buffers, and I'm not sure why it's even needed there. The only reason I could think of would be to optimize cache misses, but that doesn't seem right.

Rossmaxx · 2024-05-29T16:00:38Z

The MemoryHelper::alignedMalloc function that is changed in this PR could be replaced with std::aligned_malloc now.

I hope this is a drop in replacement. Btw it's std::aligned_alloc.

Rossmaxx · 2024-06-21T09:46:39Z

The MemoryHelper::alignedMalloc function that is changed in this PR could be replaced with std::aligned_malloc now.

Opened a PR at #7335 . Thanks for the heads up.

PhysSong · 2024-10-06T03:28:36Z

I removed some irrelevant change and then updated the branch. This PR will still require some minor changes, I guess.

Rossmaxx · 2024-10-06T04:12:34Z

benchmarks/CMakeLists.txt

+	benchmark.cpp
+)
+
+# TODO replace usages of include_directories in src/CMakeLists.txt with target_include_directories


I believe this todo can be done.

Or are you referring to #7255 ?

PhysSong reviewed Jun 27, 2018

View reviewed changes

PhysSong reviewed Aug 3, 2018

View reviewed changes

include/NiftyCounter.h Outdated Show resolved Hide resolved

lukas-w force-pushed the refac/memory branch from 2c8be68 to 7434bd5 Compare August 3, 2018 12:03

PhysSong mentioned this pull request Aug 6, 2018

Enable sample track recording #3947

Closed

16 tasks

PhysSong reviewed Aug 7, 2018

View reviewed changes

src/core/NotePlayHandle.cpp Show resolved Hide resolved

PhysSong added the in progress label Oct 26, 2018

tresf mentioned this pull request Feb 9, 2019

Update rpmalloc #4752

Closed

lukas-w mentioned this pull request Mar 10, 2019

Prevent lmms freezing when a vst hangs #4790

Closed

jasp00 mentioned this pull request Mar 12, 2019

Additional permission regarding my contributions #3346

Closed

This was referenced Apr 23, 2019

Dragging the position fader back in song editor causes crash #4956

Closed

Fix race condition in NotePlayHandleManager #4966

Merged

PhysSong reviewed May 19, 2020

View reviewed changes

src/CMakeLists.txt Outdated Show resolved Hide resolved

lukas-w force-pushed the refac/memory branch from c2d530e to 4adbb88 Compare May 22, 2020 15:21

CMake: Build lmms shared library instead of object library

846ca17

Resolves some inexplicable linking errors on Windows. Saves us from working around incomplete CMake support of object libraries.

lukas-w force-pushed the refac/memory branch from 4adbb88 to 846ca17 Compare May 22, 2020 15:23

ryuukumar mentioned this pull request Jul 21, 2020

Reorganising LMMS, renaming editors and classes and everything in between (cont. #120) #5592

Closed

56 tasks

PhysSong reviewed Sep 14, 2020

View reviewed changes

Merge branch 'master' into refac/memory

9df27e9

PhysSong removed the needs style review A style review is currently required for this PR label Dec 11, 2020

PhysSong force-pushed the refac/memory branch from e15f2ca to 9df27e9 Compare July 30, 2022 07:06

JohannesLorenz reviewed Jul 30, 2022

View reviewed changes

include/AutomationTrack.h Outdated Show resolved Hide resolved

PhysSong added 5 commits September 21, 2024 21:58

Revert irrelevant changes

ac5b3f0

Merge branch 'master' into refac/memory

47e6a30

Always build libcds as a static library

7ce62fe

Fix namespace

7782954

Remove leftover of mingw-std-threads

f54ac45

Rossmaxx reviewed Oct 6, 2024

View reviewed changes

Refactoring: Memory management #4443

Are you sure you want to change the base?

Refactoring: Memory management #4443

Conversation

lukas-w commented Jun 21, 2018

zonkmachine commented Jun 21, 2018

lukas-w commented Jun 22, 2018 • edited Loading

PhysSong Jun 27, 2018

Choose a reason for hiding this comment

lukas-w Jun 27, 2018

Choose a reason for hiding this comment

Veratil May 4, 2020

Choose a reason for hiding this comment

lukas-w commented Aug 3, 2018

jasp00 commented Feb 22, 2019

lukas-w commented Feb 26, 2019

jasp00 commented Feb 26, 2019

lukas-w commented Feb 26, 2019

jasp00 commented Feb 26, 2019

lukas-w commented Feb 26, 2019

jasp00 commented Feb 26, 2019

lukas-w commented Feb 26, 2019

jasp00 commented Feb 26, 2019

lukas-w commented Feb 27, 2019

jasp00 commented Feb 27, 2019

lukas-w commented Feb 28, 2019

jasp00 commented Feb 28, 2019

tresf commented Mar 12, 2019

jasp00 commented Mar 12, 2019

tresf commented Mar 12, 2019 • edited Loading

lukas-w commented May 22, 2020 • edited Loading

JohannesLorenz commented Sep 5, 2020

lukas-w commented Sep 5, 2020

PhysSong Sep 14, 2020

Choose a reason for hiding this comment

lukas-w Dec 11, 2020

Choose a reason for hiding this comment

PhysSong commented Dec 11, 2020

lukas-w commented Dec 11, 2020

PhysSong commented Dec 11, 2020

Rossmaxx commented Jun 18, 2022

PhysSong commented Jul 30, 2022

Rossmaxx commented Feb 26, 2024

PhysSong commented Feb 27, 2024

TechnoPorg commented Feb 28, 2024

Rossmaxx commented May 29, 2024

Rossmaxx commented Jun 21, 2024 • edited Loading

PhysSong commented Oct 6, 2024

Rossmaxx Oct 6, 2024

Choose a reason for hiding this comment

Rossmaxx Oct 6, 2024

Choose a reason for hiding this comment

lukas-w commented Jun 22, 2018 •

edited

Loading

tresf commented Mar 12, 2019 •

edited

Loading

lukas-w commented May 22, 2020 •

edited

Loading

Rossmaxx commented Jun 21, 2024 •

edited

Loading