Hi Max (and Rui), Thanks for the changes. I've done a review and I feel that the solution is rather verbose and inefficient - creating lots of temporary objects on the stack isn't going to help performance, and the code is less readable and long term maintainable compared to the original code.
I'm not the original author of the code so like you am I have to read the code to understand it's intent, and reflect on it's behaviour in the multi-threaded case. My current inclination would is that the thrashing of this single ObjectProperty object is not only not thread safe, but also can't be efficient compared to having an specific ObjectProperty created in the Serializer constructor rather at read-time. I do wonder if there might be a neater way to implement the functionality without forcing the thrashing of this object. If we are to keep this particular style of usage of ObjectProperty then the solution to the threading problem would be to have a ObjectProperty object per InputStream, this way we at least aren't creating and destroying an object at a fine grained level. I would like feedback from Rui on his thoughts on how to tackle this issue. Perhaps a first step is simply adopt my suggestion of holding an ObjectProperty object in InputStream and adapting the PROPERTY macros to reference this. This way we can avoid requiring lots of changes throughout the serializers. Thoughts? Robert. On 28 March 2012 21:43, Max Bandazian <[email protected]> wrote: > Hi, > > The set of patched files is attached. Most of this is just changing from > PROPERTY to use a local, stack-allocated ObjectProperty. > > I also ran into an issue reading in FloatArray objects, so there's a patch > for that included as well (the switch to static_cast in HeightField.cpp). > The problem here was that dynamic_cast doesn't work across library > boundaries on Linux for classes that don't export their vtable; since > osg::Array includes a type enum, it's simple enough to just check that and > static_cast instead. > > Max Bandazian > > > On Mon, Mar 26, 2012 at 7:35 PM, Max Bandazian <[email protected]> wrote: >> >> Sure, I should have it ready tomorrow. >> >> Max >> >> >> On Mon, Mar 26, 2012 at 2:07 AM, Wang Rui <[email protected]> wrote: >>> >>> Hi Max, >>> >>> I believe your changes is necessary and hope they could be applied to the >>> OSG trunk to avoid any thread-safe problems of the osgt/osgb formats. Maybe >>> you could directly submit the modified source files instead of the diffs. >>> >>> Wang Rui >>> >>> >>> 2012/3/26 Max Bandazian <[email protected]> >>>> >>>> Hi, >>>> >>>> I've been experiencing regular segmentation faults on Linux when using >>>> the database pager heavily, and it looks to me like it's caused by a >>>> particular pattern of non-threadsafe code. The PROPERTY macro defined in >>>> DataTypes writes into a statically allocated std::string each time it's >>>> used, and since the InputStream winds up calling that macro from multiple >>>> threads with no locking mechanism, it eventually gets into a simultaneous >>>> writing scenario, overruns the string's buffer, and corrupts memory. >>>> >>>> I see that there's some code in the InputStream commented out with a #if >>>> 0, which used a stack-allocated osgDB::ObjectProperty object instead of the >>>> static from PROPERTY. I'd suggest as a fix returning to that pattern, and >>>> removing the PROPERTY macro entirely since there's no reasonable way to >>>> make >>>> it threadsafe (at least not that I can think of). I've applied that change >>>> locally to my build of OpenSceneGraph, and the segfaults have stopped. If >>>> this is a change you want to integrate, I can send a zip of the diffs >>>> (against the 3.0.1 release), or just zip up the patched files. >>>> >>>> Thanks, >>>> Max Bandazian >>>> >>>> _______________________________________________ >>>> 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 >>> >> > > > _______________________________________________ > 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
