Hi Robert and Max, The original goal of the PROPERTY macro is to read a property name string from the osgt/osgb file. The read string is only used for comparing with the preserved one and will be thrown away after that. So a temporary variable is enough for saving it. I agree that we could have a member ObjectProperty in InputStream/OutputStream to avoid threading problems. But the macro PROPERTY should be slightly changed to INPUT_PROPERTY and OUTPUT_PROPERTY so that wrapper writers could understand them smoothly.
I would try to fix the problem this weekend. Wang Rui 2012/3/30 Robert Osfield <[email protected]> > 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 >
_______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
