lmms icon indicating copy to clipboard operation
lmms copied to clipboard

Published 20 hours ago • verified publisher icon LMMS

Refactoring: Memory management

Open lukas-w opened this issue 6 years ago • 48 comments

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.

lukas-w avatar Jun 21 '18 12:06 lukas-w

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

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

zonkmachine avatar Jun 21 '18 21:06 zonkmachine

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

lukas-w avatar Jun 22 '18 14:06 lukas-w

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

lukas-w avatar Aug 03 '18 12:08 lukas-w

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.

jasp00 avatar Feb 22 '19 19:02 jasp00

I already stated my permission to re-license all my past contributions in https://github.com/LMMS/lmms/issues/3412#issuecomment-285044419, 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.

lukas-w avatar Feb 26 '19 08:02 lukas-w

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?

jasp00 avatar Feb 26 '19 15:02 jasp00

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.

lukas-w avatar Feb 26 '19 15:02 lukas-w

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.

jasp00 avatar Feb 26 '19 15:02 jasp00

What exactly is ambiguous in your eyes?

lukas-w avatar Feb 26 '19 16:02 lukas-w

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

jasp00 avatar Feb 26 '19 16:02 jasp00

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.

lukas-w avatar Feb 26 '19 16:02 lukas-w

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.

jasp00 avatar Feb 26 '19 16:02 jasp00

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.

lukas-w avatar Feb 27 '19 08:02 lukas-w

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.

jasp00 avatar Feb 27 '19 11:02 jasp00

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.

lukas-w avatar Feb 28 '19 22:02 lukas-w 

 * 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".

jasp00 avatar Feb 28 '19 22:02 jasp00

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:

  1. Scalable to other projects, not just LMMS
  2. Compatible with our current license
  3. 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.

tresf avatar Mar 12 '19 16:03 tresf

  1. Compatible with our current license

Thanks. Double standard will make my life easier.

jasp00 avatar Mar 12 '19 17:03 jasp00

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".

tresf avatar Mar 12 '19 17:03 tresf

Rebased on top of master another time, applied @PhysSong's suggestions to move BufferPool::clear to MixHelpers and to fix the MSVC warning. I took the opportunity to replace clear() by a portable implementation.

lukas-w avatar Aug 27 '19 12:08 lukas-w

@lukas-w Is it ready for review, or do you need to add something more?

PhysSong avatar Oct 23 '19 02:10 PhysSong

@PhysSong This is ready for review, I haven't had time to fix CI though.

lukas-w avatar Oct 23 '19 08:10 lukas-w

@lukas-w Is the commit Fix macOS linking problem still relevant? And the changes in Ubuntu-MinGW-W64.cmake, too.

PhysSong avatar Nov 04 '19 06:11 PhysSong

Also, are there any plan to add some more benchmark suites? If so, I think having separate main.cpp is better.

PhysSong avatar Nov 04 '19 06:11 PhysSong

@PhysSong I somehow missed your review, sorry!

Is the commit Fix macOS linking problem still relevant? And the changes in Ubuntu-MinGW-W64.cmake, too

I'm not sure what you're asking. Are there any changes that would make it irrelevant that you can point me to?

Also, are there any plan to add some more benchmark suites?

Not yet. We can refactor benchmark.cpp when there's a need for it. We could also just remove the benchmark form this PR, I don't mind.

lukas-w avatar May 04 '20 16:05 lukas-w

:robot: Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! :tophat:

Windows

Linux

:robot: 
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://11511-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.73%2Bg9df27e9e8-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11511?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://11513-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.73%2Bg9df27e9e8-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11513?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/334u7783obdim3wc/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36779849"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/a7x7xwvcxyny9n6g/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36779849"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/jkrghpkftljiadk5/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36779850"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/16cu92fnqsd879bt/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36779850"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/8a1006fc-d09d-41bb-8f74-faab6b9e9a59/artifacts/0/lmms-1.3.0-alpha.1.73+g9df27e9e8-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17862?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/915baee5-f00c-4b3e-8627-eb245a099902/artifacts/0/lmms-1.3.0-alpha.1.73+g9df27e9e8-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17863?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/4bfmogfw436uv90d/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44324289"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/m9q0132yvbq67g4t/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44324289"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://11514-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.73%2Bg9df27e9-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11514?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "9df27e9e8e5ceba9177a08bbf2a57a4d19ccf78e"}

LmmsBot avatar May 04 '20 18:05 LmmsBot

Does anyone have an idea why CI is failing? AppVeyor is talking about building cds.lib and then continues to complain that libcds-amd64-dbg.lib does not exist:

LINK : fatal error LNK1104: cannot open file 'libcds-amd64-dbg.lib' [C:\projects\lmms\build\src\lmms.vcxproj]

Tests on Travis just crash on exit:

********* F*** Error in `ini./tests/testss': hcorrupted double-linked liste: 0xd0000000001062800 testing of AutomationTrackTest *********
<< 0 out of 5 test suites failed.
/home/travis/build/LMMS/lmms/.travis/linux..script.sh: line 15: 29694 Aborted                 (core dumped) ./tests/tests

lukas-w avatar May 06 '20 12:05 lukas-w

Does anyone have an idea why CI is failing? AppVeyor is talking about building cds.lib and then continues to complain that libcds-amd64-dbg.lib does not exist:

LINK : fatal error LNK1104: cannot open file 'libcds-amd64-dbg.lib' [C:\projects\lmms\build\src\lmms.vcxproj]

Tests on Travis just crash on exit:

********* F*** Error in `ini./tests/testss': hcorrupted double-linked liste: 0xd0000000001062800 testing of AutomationTrackTest *********
<< 0 out of 5 test suites failed.
/home/travis/build/LMMS/lmms/.travis/linux..script.sh: line 15: 29694 Aborted                 (core dumped) ./tests/tests

You have a libcds submodule in src/3rdparty. Does that belong?

Veratil avatar May 06 '20 16:05 Veratil

@Veratil yes, see the PR's description:

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

lukas-w avatar May 06 '20 16:05 lukas-w

@Veratil yes, see the PR's description:

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

Ah, sorry! The way your comment is worded sounded like you had no idea what it was and why it's there.

Looking back through the AppVeyor and Travis history, every build has failed with that error. :(

Veratil avatar May 06 '20 16:05 Veratil