Hi Gerrit, hi Allen,
Gerrit Voss wrote:
>
> 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 ;-)
OK. I hope it won't be too much of a bullet.
> 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 ;-)
I think I just mixed up RC and FB. But yes, it is confusing. :)
> 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.
I don't see that making a big difference right now, but I assume I will
run into that.
> 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.
>
> 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.
Hm, so you would only keep aspect-specific refcnts? And would you
actually delete the local copies when their refcnt goes to 0, and remove
them from the AspectStore? The AspectStore would then be deleted when
all aspects are removed.
I guess the CL sync would keep all the refcnts consistent. We just need
to be able to handle situations where you sync into an aspect where the
FC has been deleted already. And if we can handle that, we can handle
many other things that might be interesting: having FCs only in some
aspect (only create them when they're synced into an aspect) and have
different numbers of aspects for different FCs.
> But I still would
> prefer to keep the resovledLinks/destructor split.
I can only second Allen's request for a little explanation of that.
Thanks! :)
> 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.
boost takes care of that without locks, which is pretty neat. There is
probably still overhead on the processor level, so it might be worth
avoiding.
> Anyway you need per aspect
> refcounting to avoid ping pong syncs to destroy containers correctly.
Slightly more details? I've never been this deep inside the FCs, so I
might have missed simething.
> 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.
OK.
> 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 ;-)
I just want to avoid using them for parameters, I only want to use them
for storage.
> 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.
Yeah, the parent linking is an issue. Can we have special Fields that
take care of parent linking in assignments? Parent linking really is a
big issue right now, Allen is having heaps of trouble in his app.
> So I'm still thinking about exposing writable pointer fields again,
> but you will have to modify the field structure for that.
Writable in the reference sense? I don't know, do we really need that
instead of having a method?
>> - 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 ??
Yup. I want assignment to be ownership transfer. That way you can just
create a RefPtr, pass it out of the method as an AutoPtr and assign it
to another RefPtr without having to touch the refcnt.
> 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());
Hm, good point. Given that the pointers get smaller anyway I'm not too
worried about that.
> hmm ok it really looks like I managed to confuse you ;-).
Told ya! ;)
> 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 ;-).
Sounds good! :)
How is refcnt really handled right now? I know there is stuff in RC, but
there is also the magic MemObject in Base.
> 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.
See above.
> As > 95% is cleanup I don't see anything really evil ;-))
Well, there is cleanup and there is CLEANUP. This one is the latter...
;) It will change a very large part of the code.
> I'll think about it a little bit and if I find something before being
> cut off I'll let you know.
OK.
> 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 should be able to spend the few hours it should take.
Whatever you can do to make my life easier would be welcome. ;)
Allen Bierbaum wrote:
>
> 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. :)
...and it's not even Halloween yet! ;)
> 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.
The procs these days have special opcodes for that. But I agree, there
will be cache invalidation traffic that we might want to avoid.
> Are you saying that internally to something like an RTA you may just
> deref the RefPtr and pass around the cptr directly?
Yes.
> 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).
Given that all the aspects have to point to the same AspectStore, doing
that should be trivial. The RefPtrs won't really store any extra data.
> 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.
Hm, is that really a problem? I've never across a case where the
ownership of Actions or other things ever mattered, it was always very
clear when it needed to be destroyed. Do you have an example where this
bit you?
>>> - 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?
Right now you have the getElemP as the central point that every
dereferencing goes through. Switching to cptrs will remove that, so I'm
not sure how you can check it. If you can depend on everything using
RefPtr you could use its deref for it, but that is optimistic unless we
really forbid people/app writers from using cptrs directly.
> 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.
True. But I'm not sure if I want to force people to use RefPtrs
everywhere/forbid cptrs, as having to use RefPtrs for passing around
parameters scares me.
> 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.
I'll try, but the 'everything' part sounds very labor-intensive, and I
don't know if the few days that I have are enough for that.
Thanks
Dirk
-------------------------------------------------------------------------
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
