Hi,
If I'm understanding this correctly, the idea here would be to add an
ObjectProperty member to InputStream, add an accessor to get a reference to
it, and use that instead of allocating one per read function in the
osgWrapper classes. That seems sensible to me, although I won't have time
to do any work on it myself for a couple of weeks.
Personally I'd rather see the PROPERTY macro stay removed, though - it
doesn't really save much code (is.property().proto("Time") vs. PROPERTY(is,
"Time") for example), and since it currently lives in the DataTypes header
but would only make sense in conjunction with an InputStream, I think it
would be a bit weird from a modularity standpoint.
Max
On Thu, Mar 29, 2012 at 12:22 PM, Robert Osfield
<[email protected]>wrote:
> 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
