Hi Matthias, I have just reviewed your submission and as things stand feel that this feature is probably best implemented in a slightly different way. In general I much prefer runtime control of options like that than compile time as compile time forces users to build and distribute their own version of the OSG to take advantage of the niche feature, and also this niche usage also confers niche testing, and the less testing their is the more likely a build or runtime error is introduced without the community spotting.
My preferred approach would be to have a setting just like osg::NotifySeverity that enables/disables the per thread streams. I would also be inclined to hide the different implementation with the NotifySingelton class where their is a std::stream* getNoticeStream() and std::string* getNullStream(), this way the public C functions that pass back the return streams can just directly call the NotifySingleton and not have to worry about the implementation details of which stream to use. I also also pondering if a template implementation might be possible for thread local storage might be useful. I haven't yet thought deeply about these topics, I'll let yourself and others chip with your own thoughts before the next steps are taken. Cheers, Robert. On 31 May 2013 09:28, Matthias Schütze <[email protected]> wrote: > Hi there, > > I'm referring to a forum discussion dealing with a multi-threading > issue when using the notification API heavily. I'm sorry that it is > over half a year ago (original post: > http://forum.openscenegraph.org/viewtopic.php?t=10533). > > In the forum thread mentioned above, I stated a potential race > condition around the global static instance of osg::NotifyStream > defined in Notify.cpp. Hence, I made a suggestion to workaround this > issue by using a thread local storage: > >> Based on the svn trunk version of Notify.cpp, I experimented with another >> approach. The solution is prototype, however, here is what worked for me: I >> used a thread local storage to separate access to osg::NotifyStream for each >> thread. As discussed earlier in this thread, this assumes stream classes >> (including std::ostream and std::stringbuf) to be at least reentrant. >> According to >> http://msdn.microsoft.com/en-us/library/c9ceah3b%28v=vs.100%29.aspx this is >> true at least for my Visual compiler implementation. >> Currently, there are some other drawbacks with this solution: As I used >> QThreadStorage for now, it requires QtCore library to be linked in core osg. >> Maybe, another TLS implementation would be more suitable (does OpenThreads >> has one?). Although I could not recognize capital performance loss yet, this >> may be another critical issue. >> Nevertheless, I tried the modified version upon OSG 3.0.1 on all my Win 7 >> x64 machines and the heap corruption never showed up again. For further >> inspection and interest, I attached my modified version of Notify.cpp which >> has a preprocessor flag OSG_NOTIFY_STREAM_PER_THREAD to toggle the use of >> thread local storage. > > Robert then requested, if it is possible to use OpenThreads instead of > QThreadStorage for the job: > >> OpenThreads doesn't expose thread local storage but does use it internally >> to provide the Thread::CurrentThread() functionality. Might it be possible >> to use this in your implementation? > > Finally, I followed this hint to implement a local class > StreamPerThreadStorage which is independent from Qt. I attached a > version of Notify.cpp based on the current trunk version. This is > tested on a Win 7 x64 machine where the former heap corruption did not > show up again. > > Again, there is a preprocessor flag OSG_NOTIFY_STREAM_PER_THREAD. > Currently, its definition is hardcoded (and commented out in the > attached file). If my changes would be submitted, maybe one could > expose the flag to CMake in order to make the use of thread local > storage optional. Unfortunately, I have no clue about CMake > programming. > > Kind regards, > Matthias Schütze, Germany > > _______________________________________________ > osg-submissions mailing list > [email protected] > http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org > _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
