[Ns-developers] A question about downcast netdevice pointer.

[Gustavo Carneiro]

2008/9/30 Gustavo Carneiro <gjcarneiro at gmail.com>

>
>
> 2008/9/29 <craigdo at ee.washington.edu>
>
>>
>> > *sigh* why do you always have to write such long replies... :P
>>
>> As an aside, that particular smiley may be interpreted by (at least)
>> Americans in ways you don't intend.
>
>
> The meaning I intended to convey is that I am being somewhat lazy by not
> wanting to read big emails like yours.  So you should take my complaint only
> half seriously.
>
>
>>
>>
>> Anyway, typing long emails is not my favorite thing to do, especially when
>> I
>> could be out enjoying myself or doing more productive work.  I write these
>> long emails because it is obvious, based on the replies that I get, that I
>> am not being understood.  I assume that this is because of communication
>> problems.  When this happens, I try to express what I'm talking about from
>> different viewpoints using different examples, hoping that I can come up
>> with something that will connect.  I have found this helpful in the past.
>> It may be the case that you find reading all of this English painful and
>> I'm
>> actually being counterproductive.  I don't know.  I do know that if I was
>> trying to read all of this in Portuguese or French, it would be bad (TM).
>> If you have a better way, please, please, let me know.
>
>
> The problem with long emails is that not many people have time to read
> them.  You might not get any objections to your long emails, but it could
> some times mean no one had time to read it, not always that they agree with
> it.
>
> I suggest you think of email as a presentation, with bullets.  Focus on a
> few key ideas, and add just a little text around them, but not too much.
>
>
>>
>>
>> [ ... ]
>>
>> > Here's what I think (and I wish you could be as objective as I try to
>> be):
>>
>> I don't know why you think I'm not being objective.  As I said above, I'm
>> trying to communicate what I think is an important point.  I hope you
>> realize I am honestly trying to communicate here.  I'm assuming you are a
>> good guy who is trying to do his best at this ns-3 stuff as well.
>>
>> >   1- You point out problems that AggregateObject should be
>> > detecting and
>> > warning the user, not GetObject.  Swapping dynamic_cast for
>> > GetObject would
>> > not help in any additional way that proper assertions in
>> > AggregateObject
>> > can't detect;
>>
>> This is why I write so many of those words.
>>
>> I'm *not* trying to draw an operational distinction between how
>> dynamic_cast
>> and GetObject work in the case you describe.  I never have.  I've already
>> said that the results of dynamic_cast and GetObject are identical in the
>> simple, constrained case you mention.  I think I've said it several times.
>>
>> I'm talking about the conceptual model people have when they are using the
>> ns-3 Object system and how the APIs hang together.  What I'm pointing out
>> is
>> that by thinking about the inheritance case of ns-3 Objects as somehow
>> different, it leads to thinking about the system in ways that allow other
>> errors to creep in and makes clients think about implementation
>> dependencies
>> for almost no reason.
>>
>> One of the things I am trying to convey is that it should really, really
>> be
>> obvious that:
>>
>> > > Ptr<Wifi> wifi = CreateObject<Wifi> ();  // 0x123 address
>> > > Ptr<Csma> csma = CreateObject<Csma> ();  // 0x456 address
>> > > wifi->AggregateObject (csma);
>>
>> would be a programming error.  This is obvious if you think of the
>> situation
>> as Objects, but is not so obvious if you think of it as having some
>> Objects
>> and a separate dynamic_cast part.  I think the fact that you asked what
>> GetObject would do, instead of flatly saying that this is an error
>> situation, reinforces my contention that there is an issue in thinking
>> about
>> dynamic_cast between ns-3 Objects as somehow different and special.
>>
>> I think that the fact that the system works in the presence of this kind
>> of
>> aggregation indicates that there was a similar distinction made during
>> implementation, and that this led to the kind of programming error above
>> as
>> being legal in ns-3 today.
>>
>> >   2- I am all in favor of AggregateObject detecting errors;
>> > in fact, I was
>> > the first to suggest it:
>> > http://www.nsnam.org/bugzilla/show_bug.cgi?id=162
>>
>> We are all in favor of AggregateObject detecting errors, of course.
>>
>> The issue I am attempting to point out is that if you think of the objects
>> in question (CsmaNetDevice and WifiNetDevice) as somehow not possibly
>> being
>> part of an aggregation and think about moving between NetDevice and
>> CsmaNetDevice through a dynamic_cast, it is not immediately obvious that
>> an
>> error even exists.
>>
>> If you think of the object model in terms of, "[having] GetObject to do a
>> downcast is just convenient syntactical sugar" and "we have normal C++
>> class
>> single inheritance with a very simple dynamic object aggregation facility"
>> how can you immediately come to the conclusion that your example has a
>> very
>> serious error?  I don't think you can.
>>
>> That is the core of the point I'm trying to make!  It's got nothing to do
>> with taking C++ inheritance away or requiring massive API changes.  It's
>> about how looking at the issue slightly differently makes the global
>> system
>> a lot easier to understand and explain, and be less error-prone.
>>
>> >   3- I am not aware of any situation where a downcast via
>> > dynamic_cast would
>> > not work 100% correctly unless there is already an underlying
>> > aggregation
>> > error which would/should be detected by AggregateObject instead;
>>
>> If I decide for some reason to implement a power model for a net device, I
>> could implement it using inheritance in my net device or using aggregation
>> with my net device.  If I implement this model using inheritance, I could
>> use dymanic_cast to get to my power model "interface."  If I use
>> aggregation, DynamicCast would not work in this situation where there is
>> no
>> error detected by AggregateObject.
>>
>> GetObject works in both cases.
>
>
> True, but this does not make a lot of sense to me.  The same person that
> implements the power model knows how to access it.  And of course you cannot
> change the way something is implemented without changing the way that code
> accesses the implementation.
>
>
>>
>>
>> >   4- On one hand you say developers should pretend a subclass
>> > is completely
>> > independent from its base class, like they're two different objects
>> > aggregated together.  In that case, how do you explain the
>> > code (e.g.) in
>> > packet-socket.cc:
>>
>> No, not at all.  I'm saying that it would be helpful if they used
>> GetObject
>> to move between two Objects even if they happen to be implemented as base
>> class and subclass.
>
>
> It might be OK if we don't know any better.  But if you _know_ one is a
> base class of the other, I see absolutely nothing wrong with using dynamic
> cast.
>
>
>>
>>
>> > int
>> > PacketSocket::Connect(const Address &ad)
>> > {
>> > [...]
>> >   NotifyConnectionSucceeded ();
>> >   return 0;
>> >  error:
>> >   NotifyConnectionFailed ();
>> >   return -1;
>> > }
>> >
>> >   Should we be forced to write GetObject<Socket>
>> > ()->NotifyConnectionSucceeded () ?
>>
>> No.  I said you should absolutely not have to do this *very* *explicitly*
>> in
>> my last email.
>>
>> There are two points of view here.  Your perspective here seems to be from
>> the implementation of the PacketSocket.  The packet socket implementation
>> wouldn't have any reason to use GetObject.  It has defined an "interface"
>> as
>> being Socket + PacketSocket extensions.  It just implements PacketSocket
>> using inheritance and virtual methods.  No change.
>>
>> I'm talking about what happens when someone *uses* a packet socket.
>> Presumably they somehow get a pointer to a Socket and want to get at a
>> PacketSocket.  A PacketSocket "interface" is *defined* as having all of
>> the
>> methods of the Socket plus the additional methods provided by class
>> PacketSocket.  You could socket->GetObject<PacketSocket>() and have access
>> to all of the "Socket + PacketSocket" methods or you could
>> DynamicCast<PacketSocket>(socket) to get access.  What I'm saying is that
>> for other reasons not directly related to the specifics of this case, it
>> would be preferable to *encourage* people to use the GetObject version.
>>
>> As I've said several times, I'm not suggesting, and I have never
>> suggested,
>> that we must refactor the PacketSocket to exclude the Socket methods and
>> make it so the dynamic_cast would not work.  I have not said that we need
>> to
>> rewrite PacketSocket.  I'm not quite sure how we got here, but here's what
>> I
>> said last week reiterating the same point:
>>
>> "What I said was that by using GetObject instead of dynamic_cast, you
>> don't
>> have to use different operations to move between objects that were
>> aggregated together explicitly, versus objects that were associated
>> together
>> by a different implementation (inheritance)."
>>
>
> If the programmer is well aware of the difference, I don't see why using
> different accessors would be bad.  In fact, it might make the code more
> readable.  Where I see dynamic_cast being used I know that the pointer is
> being tested against a subclass.
>
>
>>
>>
>> If you had implemented PacketSocket as two completely separate
>> "interfaces"
>> aggregated together you would be *required* to move between them using
>> GetObject.  If you had implemented PacketSocket as "PacketSocket inherits
>> from Socket," you could use GetObject to move between them OR you could
>> use
>> dynamic_cast.
>>
>> GetObject works in both cases and there's only one consistent easy to
>> explain model to consider and remember from the client perspective.
>>
>> >   5- Finally, you claim there is no downside to using
>> > GetObject instead of
>> > dynamic cast.  That is debatable for two reasons.  One,
>> > because developers
>> > already know C++ and know how to use dynamic_cast much better than
>> > GetObject, and two because I am not aware of performance
>> > comparison between
>> > dynamic_cast and GetObject.  GetObject might be much slower.
>> > When in doubt
>> > I prefer to use dynamic_cast, as it is generated by a
>> > compiler with decades
>> > of of experience, hence likely to be better optimized.
>>
>> I claim that thinking about using GetObject is *helpful* for the reasons I
>> have outlined; and there is no API change required in doing so.  I claim
>> that the presence of ns-3 Objects in a C++ inheritance situation does make
>> a
>> difference that needs to be considered.  I claim that the current
>> situation
>> is hard to define and understand if you are looking at it from a point of
>> view that admits aggregation.  I claim that the performance difference in
>> implementation between GetObject and DynamicCast could be only a function
>> call (i.e. the dynamic_cast happens first in the GetObject and if it
>> succeeds that is all that happens).
>
>
> In case the dynamic cast would fail, maybe the dynamic_cast returns NULL
> with much less work than GetObject has to do to iterate over all aggregated
> objects, and for each object call GetTypeId(), traverse the typeid
> inheritance path towards the root type.  I think you underestimate the
> complexity of GetObject.
>
>
>> I'm not demanding that you, personally, do anything.  If you recall, what
>> I
>> wrote was: "If we are going to pronounce a best practice, I think
>> GetObject
>> should be that practice [...]."  You can always do whatever you prefer.
>
>
> I am still not convinced.  The crux of your arguments appears to be that
> the programmer is often unsure how types are related, whether by aggregation
> or inheritance, or that the code often switches between one or the other
> style of relationship.  In that case, always using GetObject makes sense.
> But I don't agree that the programmer does not know exactly what is going
> on, and I don't think relationships will change (or at least they shouldn't,
> API stability FTW!).  So recommending GetObject as "unconditional best
> practice" seems overkill to me.
>
> Maybe the recommented best practice should be more flexible something like
> this:
>
>     "DynamicCast works when the programmer has a base type pointer and is
> testing against a subclass pointer.  GetObject works when looking for
> different objects aggregated, but also works with subclasses, in the same
> way as DynamicCast.  If unsure, the programmer should use GetObject, as it
> works in all cases."
>
> I just don't want to say "Use GetObject always" as if dynamic cast was
> somehow tainted.  dynamic cast _is_ appropriate if you know what you're
> doing.  GetObject is probably much slower than dynamic cast; at least that
> is what I suspect until proven wrong.  Sorry for being pessimistic.
>

I went ahead and did some benchmarking (patch attached).  dynamic_cast is
roughly twice the speed of GetObject.

-- 
Gustavo J. A. M. Carneiro
INESC Porto, Telecommunications and Multimedia Unit
"The universe is always one step beyond logic." -- Frank Herbert
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bench.diff
Type: text/x-diff
Size: 2921 bytes
Desc: not available
Url : http://mailman.isi.edu/pipermail/ns-developers/attachments/20080930/18c15115/bench-0001.bin