lballabio
simplify singleton implementation
Following the suggestion from @sweemer in https://github.com/lballabio/QuantLib/pull/1368. As far as I understand: We require C++11 now and this implementation is indeed thread-safe in C++11. I am currently testing this in our framework, but wanted to put that out for discussion in parallel. @klausspanderen @lballabio
I am assuming that there is no use case for several "sessions" from a single thread, i.e. that the concept of sessionId() becomes superfluous.
There are more changes to come, if we agree on the changes to singleton.hpp.
Coverage decreased (-0.004%) to 71.406% when pulling e06ae2203235f3d0c0b0c6c2bc28381f74649c38 on pcaspers:simplifyThreadLocalSingleton into b942a80d4401513414e4942cc11226d2efb4f44d on lballabio:master.
Agreed, C++11 simplifies the whole thing.
I think an hypothetical use case for sessionId() could be to try and map language-level thread when QuantLib is exported to other languages via SWIG; but in C# they map to C++ threads so they're covered already, and in other languages sessionId() is probably not the right interface anyway.
In any case, we might ask on the users mailing list if people are using it in ways we don't see.
Much cleaner indeed and always better when we can leverage the guarantees provided by the language.
On that note, we might want to take advantage of this opportunity to add some comments in the source code on how to use sessions correctly. Namely, while the initialization of singletons (both global and non-global) is guaranteed to be thread-safe when sessions are enabled, the subsequent access to singletons is not guaranteed to be thread-safe in any way, and so users must supply their own synchronization wherever required. This might be an obvious point to most people, but I think it can't hurt to provide a bit of guidance to people who aren't that experienced with multi-threaded programming.
Also we should probably add a preprocessor message or warning whenever QL_ENABLE_SINGLETON_THREAD_SAFE_INIT is defined to let people know that it is now always enabled by default and no longer needs to be defined.
Yes that makes sense, I'll add some comments in that spirit.
As for QL_ENABLE_SINGLETON_THREAD_SAFE_INIT: I'd just remove that macro entirely. At least cmake will give a warning if the configure step is invoked with this variable defined. Not sure about autoconf / automake.
@lballabio the failing test seems to use a test2.cpp source file with a sessionId() implementation referencing the type ThreadKey. Where is this file set up, I assume that is generated on-the-fly for the test?
It's generated in .github/workflows/linux.yml, as well as in linux-nondefault.yml and linux-full-tests.yml in the same folder. You can remove the line referencing sessionId.
Namely, while the initialization of singletons (both global and non-global) is guaranteed to be thread-safe when sessions are enabled, the subsequent access to singletons is not guaranteed to be thread-safe in any way, and so users must supply their own synchronization wherever required.
You mean for the global ones? Because otherwise, when sessions are enabled, each thread has its own instance.
I mean mainly between the global singleton and the thread-local ones. Presumably there's a use case for both global and thread-local singletons to coexist in the same process, and in that case all access to the global singleton from the other threads needs to be properly synchronized in the user's application.
@lballabio should we move on with this, there was no reply to my question on the mailing list, so I suppose we won't break anyone's code with that
BTW we are using that new singleton implementation now in a multithreaded QL environment and it works fine so far.
I'd deprecate sessionId in the release notes of 1.27 (which might reach more people?) and merge this after the release.
Can we try to merge this soon? There's something I'd like to contribute on top of it.
I've deprecated the old sessionId implementation in the release notes for 1.27 and we usually wait a couple of releases for removal. I'd merge this after I release 1.28 at some point in October.
Sorry I read https://github.com/lballabio/QuantLib/pull/1377#issuecomment-1154095135 as saying that this would be merged in 1.28, my bad.
What if we merge in two stages - the thread_local stuff in 1.28, and then removing the sessionId function prototype in 1.29?
Based on the lack of reply to Peter's email to quantlib-users, there's no known use case for mapping multiple threads to the same sessionId, so this change should be transparent to users and not require any user code changes. Or is the idea to give people who don't subscribe to the list time to read the release notes for 1.27 and schedule any necessary refactoring?
In fact, even removing the sessionId function prototype should not require any user code changes, as any definitions in user code will simply not be linked into the library. Unless users read the release notes, they may never know to clean up their sessionId definitions - or am I missing something?
I can submit a pull request with the changes for the first stage if that sounds fine to you.
Yes, it's about giving people another chance at reading the release notes — I'll reiterate the upcoming change in the ones for 1.28. The interface doesn't change, but we're changing the behavior so I prefer to give another heads-up.