Hello Edmund,
Edmund Buchal wrote:
>> - OSGFieldContainerFactoryImpl.{h,inl}:
>
>> you implemented id reuse in there and switched from using a std::vector
>> to a std::map for the FieldContainerStore.
>> Reusing ids is problematic, as there is code that (implicitly) relies on
>> ids increasing monotonically. On the other hand, if you *do* reuse ids,
>> I see no need to use a std::map instead of the vector, as the reuse will
>> keep the vector bounded by the (maximum) number of simultaneously
>> existing FieldContainers.
>> If the growth of the FieldContainerStore really is a problem for you
>> application, I would prefer to switch to a hash_map and not reuse ids.
>> Your comment in the code says you did not get hash_maps working, what
>> was the problem you had with those ?
>
> The ids should still grow continuously the way i implemented it (the id is
> incremented every time a new container is created, see
> OSGFieldContainerFactoryImpl.inl, line 118), the map is used to store pairs
> of (id, container) in order to be able to access the containers by id later.
> Using hash maps would provide faster access to the containers, but the code
> did not compile under linux/gcc.
> I made the change because the parser seemed to grow the container vector
> continuously thus causing increasing memory use with every loaded file.
uhm, sorry, but no, you are reusing ids:
in FieldContainerFactory::registerContainer:
if(_mUnusedIds.size() == 0)
{
//if we have no ids left that are no longer in use create a new one
_pFieldContainerStore->insert(
FieldContainerStorePair(++_mMaxUsedId, pFieldContainer));
returnValue = _mMaxUsedId;
}
else
{
//if we have ids left that are no longer in use take those
returnValue = _mUnusedIds.top();
pFieldContainerStore->insert(
FieldContainerStorePair(_mUnusedIds.top(), pFieldContainer));
_mUnusedIds.pop();
}
and in FieldContainerFactory::unregisterContainer:
if(_pFieldContainerStore != NULL)
{
_mUnusedIds.push(pFieldContainer.getFieldContainerId());
_pFieldContainerStore->erase(pFieldContainer.getFieldContainerId());
}
anyways, I'll try to switch the factory to using a hash map and see if
that has not too big of a performance impact later this week.
>> OSGFieldContainerPtrDepImpl.inl:
>
>> you changed the deleteContainers function to call destroy for each
>> aspect, which is not correct. Destroy should only be called once per
>> container, while destroyAspect should be called for each aspect copy of
>> a container.
>> If this fixed a mem leak for you, it is likely that some FieldContainer
>> does not have a correct implementation of these functions and that is
>> the place that should be fixed. Do you have information on which
>> container type caused the leak ?
>
> I will have another look at that one, i'm not very familiar with the way
> aspects work yet.
ok, thanks.
>> OSGRemoteAspect.cpp:
>
>> you removed definitions of two overloads of getFieldFilter, why ?
>
> I did not remove anything, just changed the createCurrentStateChangeList
> method to work with the FieldContainerFactory changes i made, maybe I had an
> older version of this file?
>
>> OSGVRMLFile.cpp:
>
>> this seems to do the same as before, am I missing something ?
>
> line 160: i changed
> //delete [] (*it).first;
> to
> delete [] (*it).first;
>
> There was a comment that the delete did not work under linux for some reason,
> however it worked fine for me.
sorry, please ignore these last two items. It turns out I have local
changes in these files and they showed up when I diff'd them against
your modified files; my apologies for the noise.
Thanks,
Carsten
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference
Don't miss this year's exciting event. There's still time to save $100.
Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
Opensg-users mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensg-users
