Gerrit Voss wrote:
> Hi,
>
> On Fri, 2007-03-30 at 19:10 -0500, Dirk Reiners wrote:
>> Hi Gerrit (and anybody else who wants to comment),
>>
>> it fits well that you're out of town next week, that means you're not
>> working on
>> OpenSG much. :) Can you make sure that you have all your changes committed,
>> especially in the FC?
>
> hmm, I don't think it would be a good idea to push my tree and than run
> away, so I guess I have to bite the bullet and redo it after your
> changes. Anyway I wanted to clean it up for a long time so it might be
> a good opportunity ;-)
>
>> As it happens, my wife is out of town next week, too. Which gives me a good
>> chance to get some work done on OpenSG, and I would like to get the dreaded
>> FC/CPtr thing done. At the same time, I would really like to simplify the FC
>> code significantly, as, frankly, the current version is just scary.
Agreed. There is a need for a cleanup. I wish you all the luck in the
world and I hope it works out for you as that code scares me every time
I look at it. :)
>
>> So the goals are:
>> - simplify the code. Remove the mixins and use simple derivation as far as
>> possible. And don't worry about a little bit of code duplication, if it
>> keeps
>> things more understandable. Code is not just for compilers, it's for people,
>> too. ;)
>
> And I just hacked fcd2code to deal with mixins ;-). But as I even got
> you confused (the FC is actually derived from RC) it seem to be time for
> a change ;-)
>
> Anyway a lot of the mixins came to be because of the FC structure and
> the custom pointers. Switching everything back the CPtr should eliminate
> a lot (all) of these without code duplication. Especially being able to
> use ReflexiveContainer * for FieldContainer and FieldBundle makes the
> difference.
>
>> - switch to cptrs only. I don't think the code will change much from the
>> current
>> CPTR mode: have an AspectStore refed from the class and use that to keep the
>> RefCount
>
> my current feeling is that we should keep the refcounter inside the
> container and don't push it to a central location. Having anything in a
> central location is what caused most of the trouble.
I agree with GV, having a central location sounds like a bad idea. It
would force a single point of thread synchronization (performance
bottleneck) and could cause major cache invalidation issues when using
multiple processes.
> As the aspect copies are now independent and only linked via the aspect
> store we actually should be able to handle containers locally as we can
> destroy a single copy independent of the other copies. But I still would
> prefer to keep the resovledLinks/destructor split.
It would be very helpful to document this split and what it means to the
user. I still don't know what I should do where. :)
>> - add weak ptrs and make ref counting more efficient. To do that I want to
>> use
>> the boost::sp_counted_base, which supports weak pointers and also has
>> specializations for lock-free thread safe counting. It's not quite the way I
>> would like it to be as you pretty much have to use a pointer to it instead
>> of
>> deriving the aspect store from it, but maybe I can figure that one out, too.
>
> as above I would not push the FC refcounting into the aspect store. As I
> did the aspect store I left it out of there intentionally. And you would
> not have to worry about thread safe locking. Anyway you need per aspect
> refcounting to avoid ping pong syncs to destroy containers correctly.
> Yes you can replicate this as it is now with the AspectStore but as one
> of your main targets is simplifying things we should use per container
> refcounting.
>
> Also if you increase the number of ref count calls and keep a single
> refcount this can cause trouble on NUMA architectures as you would trash
> a single value every processor might have in cache. So keeping as many
> items separated as you can looks better to me.
>
>> - remove the PointerBuilders, as they are not necessary any more if we don't
>> have different modes. This one will go a long way to make the code look more
>> normal.
>
>> - change the fields to use RefPtrs, remove explicit ref handling in the
>> code.
>> This does not mean using RefPtrs everywhere, just for storage.
Are you saying that internally to something like an RTA you may just
deref the RefPtr and pass around the cptr directly? In that case, I
think it makes sense but I would only do it in places that performance
really dictates it. The only place I can see this biting you is if
there is code somewhere that is passed something like a Node* and needs
to get it's NodeRefPtr back for that fc. It may be interesting to have
some code in the fc classes that could return a RefPtr from this (in the
same way boost::shared_from_this works).
> Hmm, this sounds a little bit strange to me. Either we use ref pointers
> or we don't. But I guess I wait and see what it will look like ;-)
>
>> This should also go some way towards making the reflexive interface more
>> complete
>
> I think the reflexive interface is massively suffering from not
> having vtables inside the fields rather than which pointer you use.
> As ref counting is only half of the bookkeeping (parent linking the
> other) I don't think it makes any (big) difference.
>
> So I'm still thinking about exposing writable pointer fields again,
> but you will have to modify the field structure for that.
>
>> - add AutoPtrs to avoid ref/unref for just passing out values. This should
>> avoid
>> overhead when accessing field data.
>
> Are you sure you really mean something like std::auto_ptr ??
Can you explain this one a bit more, what are you talking about here?
One thing I would really like to see is for OpenSG 2.0 to use
boost::shared_ptr's everywhere that it currently returns, allocates, and
stores non-fc's by cptrs. For example IntersectionAction::create()
could return an boost::shared_ptr<IntersectionAction> probably with a
typedefed ptr type like IntersectionActionPtr or something similar.
>
> One thing you have to take care of is that you loose the ability to
> pass pointers by reference as they are unrelated entities. So you have
> to make sure every interface that traffics a pointer must take the
> object not a reference. Otherwise compilers will complain for something
> like:
>
> takeFooPtr(getFooPtr());
>
>
>> - Use fcd2code as much as possible, to avoid having to manually mess with
>> Fields.
>
>> Structure-wise I think I'll derive RC and FC from a base that has common
>> stuff
>> (not totally sure what it's going to be, but probably not too much). The
>> reason
>> they're not derived from each other is that RC and FC probably need to
>> handle
>> RefCounting differently.
>
> hmm ok it really looks like I managed to confuse you ;-). The structure
> is
>
> ReflexiveContainer
> |
> +---------------+
> | |
> FieldBundle FieldContainer
>
> Plus a couple of mixins ;-) So ReflexiveContainer should be the base
> structure you are looking for ;-) And you can even keep the refcounting
> inside RC ;-).
>
>> Questions that I have:
>>
>> - Should we get rid of the ...Ptr types? They're not strictly necessary any
>> more, but without them Allen's memory debugging won't be possible, which i
>> think
>> is very helpful. In non-memdebug mode they would be direct CPtrs.
Unless I am missing something, we could still keep this type of thing
but make the checks live in the RefPtr code. I really really would like
to see the whole RefPtr --> FcPtr --> FC* --> FC simplified a lot and it
seems that removing the FcPtr part would definitely be a good thing.
As long as we are storing everything with RefPtrs in the new system I
don't see a problem. The parts needed for memory checking are:
- fc invalidation flags: stored in the fc itself
- Setting of the flags: done in the fc destructor or deallocation code
- Checking the flags: done in the fc ptr lookup/dereferencing code.
could be done in RefPtr
Am I missing something?
The other thing to remember is that once we start using RefPtrs to store
everything then this can become much less of an issue because it will be
impossible to hold a RefPtr to something that has been deallocated. That
simple change will remove the vast majority of memory issues because
dangling references will be no more.
>> - are the aspect-specific refcounts life? Are they needed, did they fix
>> something?
>
> yes you can not do without them. Well you can but than you have to have
> a ping pong sync between aspects to get everything destroyed properly.
>
>> - what did I miss why this is a bad idea?
I would add one more big one: include (doxygen) comments in the core
classes explaining what everything is, what the methods are for, and how
they work. IMHO one of the reasons the current code is so scary is
there are very few comments to guide the way and explain what everything
is in the code.
>
> As > 95% is cleanup I don't see anything really evil ;-))
> I'll think about it a little bit and if I find something before being
> cut off I'll let you know.
>
> On a more technical note, would it make sense that I quickly collapse
> all the mixins as I wrote them it might be quicker and give you a
> cleaner picture to start from ?.
I know I would like to see that and it would make the code easier for us
to use in the meantime. :)
-Allen
>
> I should be able to spend the few hours it should take.
>
> gerrit
>
>
>
>
>
>
>
> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share your
> opinions on IT & business topics through brief surveys-and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> _______________________________________________
> Opensg-core mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/opensg-core
>
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Opensg-core mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensg-core
