jamoma / JamomaCore

Jamoma Frameworks for Audio and Control Structure
Other
36 stars 14 forks source link

Rewrite TTHash and TTList so that they use std::mutexes #360

Closed jcelerier closed 8 years ago

jcelerier commented 9 years ago

Hello,

As a contribution to the effort toward more use of the standard c++ features in Jamoma, here is a sample rewrite of TTList and TTHash with direct use of std::mutex and std::unique_lock.

The main difference with the previous way is that instead of doing lock() and unlock(), we create a unique_lock object, either empty or with the mutex of the class at construction; the destruction of the stack variable created in auto locker = acquireLock(); calls unlock() upon leaving the method.

For performance, mutex::lock()and mutex::unlock()could be called directly (although I'd argue that the most efficient way would be to determine at compile-time if TTHash or TTListwould require thread-safety, by having them depend on a template bool parameter which would enable - or not - locking; this of course has the drawback of putting everything in headers (or forward-declaring them and defining both TTHash<ThreadSafe=true> and TTHash<ThreadSafe=false> in TTHash.cpp), but would remove the unnecessary check on each call to acquireLock).

jcelerier commented 9 years ago

As a side note, I also did some cleanup of seemingly dead and/or unused code.

jcelerier commented 9 years ago

As a second side note, building with MinGW on windows now works as of a2659de , which might be useful for using more advanced C++ features on Jamoma with Windows. I don't know at all for JamomaMax however.

tap commented 9 years ago

I agree that having the switch for thread protection occur at runtime does not really make sense.

One thing that I think should be a consideration here is to do e.g.

using TTMutex = std::mutex

because that gives us the ability to substitute mock classes for unit testing or difficult debugging.

I also agree that it would be nice to have TTHash and TTList as templates that are header only. Historically it was not possible, at because we needed to keep the internals of TTHash, which were platform dependent, hidden from code that linked to the Foundation. Now that we have e.g. std::unordered_map on all platforms this concern has evaporated.

The same is true for e.g. TTThread, by the way.

tap commented 8 years ago

@avilleret what is the conclusion on this one? looking at it I'm not clear if the branch has been merged or cherry-picked? Github seems to think it hasn't?

avilleret commented 8 years ago

@tap it hasn't been merge but the pull-request have been closed when dev branch have been removed to I will reopen it against master branch