combine UDPPacket and UDPPacketInternal

[cmcfarlen]

No description provided.

[cmcfarlen]

This was moved from UDPConnectionInternal to avoid having the Internal class in UDPPacket.

[maskit]

[approve ci autest]

[ywkaras]

Verified that UDPPacket is currently never directly instantiated: #9470 .

[ywkaras]

Here are the source files that reference UDPPacketInternal:

wkaras ~/REPOS/TS2
O$ grep -Fl UDPPacketInternal $(findsrc)
./include/tscore/UDP_stubs.h
./iocore/net/P_UDPPacket.h
./iocore/net/P_UDPNet.h
./iocore/net/QUICNet.cc
./iocore/net/P_QUICNet.h
./iocore/net/P_UnixUDPConnection.h
./iocore/net/QUICPacketHandler.cc
./iocore/net/UnixUDPNet.cc
./iocore/net/I_UDPConnection.h
./iocore/net/UnixUDPConnection.cc
./iocore/net/QUICPacketHandler_quiche.cc
wkaras ~/REPOS/TS2
O$ 

Here are the source files that reference UDPPacket but not UDPPacketInternal:

wkaras ~/REPOS/TS2
O$ grep -Fl UDPPacket $(findsrc) | while read F ; do if grep -F UDPPacketInternal $F > /dev/null ; then : ; else echo $F ; fi ; done
./include/ts/InkAPIPrivateIOCore.h
./iocore/net/I_UDPNet.h
./iocore/net/QUICNetVConnection.cc
./iocore/net/I_UDPPacket.h
./iocore/net/quic/QUICConnection.h
./iocore/net/quic/Mock.h
./iocore/net/quic/QUICPacketReceiveQueue.cc
./iocore/net/quic/QUICPacketReceiveQueue.h
./iocore/net/test_I_UDPNet.cc
./iocore/net/QUICNetVConnection_quiche.cc
./iocore/net/P_QUICPacketHandler_native.h
./iocore/net/P_QUICNetVConnection_native.h
./iocore/net/P_QUICNetVConnection_quiche.h
./iocore/net/P_QUICPacketHandler_quiche.h
./src/traffic_server/InkIOCoreAPI.cc
wkaras ~/REPOS/TS2
O$ 

So there is some scoping that is lost by combining the classes. Why is this scoping of no value, or what outweighs the value of it.

[cmcfarlen]

There is nothing to encapsulate and the only virtual function was free(which is only to support the unnecessary inheritance) so I don't see any reason for inheritance here.

[ywkaras]

Yes there is no class or namespace scoping, but looks like there is some file scoping. Good C++ avoids reliance on file scoping, but this is trafficserver (C wannabe). Just seems like something to consider before we throw it away.

[cmcfarlen]

I can't be certain but it seems like the P vs I files were split up to accommodate the TS_INLINE stuff that was removed prior. I don't see any gain in having two classes here.

[ywkaras]

I've identified a specific potential gain. If you feel I'm being unreasonable, I'd be fine if you found a different reviewer.

[bneradt]

I've thought about this for a while and I'm coming to the conviction that you are both making true statements:

1. You (Chris) are right that inheritance is not correct for this. Nothing of the interface of UDPPacket is extended by UDPPacketInternal since UDPPacketInternal is a member-only class. Further, the UDPPacketInternal is always the only allocated class, with the "internal" stuff being accessed via a cast from UDPPacket. This is kind of strange.
2. @ywkaras is right that these classes are intentionally divided via internal and external designs. This can be seen by looking at the uses of all the members of UDPPacketInternal. Both pktLength and segment_size, for instance, are in no external code. They are in UDPPacketInternal and are only used in iocore/net/UnixUDPNet.cc. The same is true for chain and delivery_time. In fact I think all the members of UDPPacketInternal, if I'm scanning things correctly, are used in "implementation" code, that is in P_ headers or UnixUDPConnection.cc or UnixUDPNet.cc files. This interface and implementation distinction is valuable.

My suggestion, rather than combining the classes, would rather be to convert the classes from an inheritance relationship to a containment relationship where UDPPacket contains a UDPPacketInternal class. This would satisfy both your observations.

[ywkaras]

That would have the same problem I think. The contents of UdpPacketInternal would be visible in files where it is not currently visible (loss of scoping).

[cmcfarlen]

That would have the same problem I think. The contents of UdpPacketInternal would be visible in files where it is not currently visible (loss of scoping).

Part of the cleanup intent was to combine the scoping so I'm not sure why that keeps coming up. I can restore the details as a separate class and fixup the visibility but there is a cost (code complexity and runtime) to having the separate scope that I don't want to restore.

[ywkaras]

To me code with less scoping is harder to understand. But it's true that, with this weird form of scoping, there is a small performance penalty.

[maskit]

I don't think we are at a good stage to redesign / optimize the structure, because we'll have changes for recvmmsg and maybe also io_uring. I'd say making the structure simple is a good first step for future changes.

[ywkaras]

Sounds like I just got outvoted.

[maskit]

No, my comment is +1 for current code on this PR. I'm saying we'll know what need to be "visible" while we work on the new features, and removing UDPPacketInternal would remove bias from current broken design.

[ywkaras]

Here's a (tricky) approach that will retain the current amount of scoping, and get rid of the virtual calls: https://godbolt.org/z/G45cro7Pz

[bneradt]

That would have the same problem I think. The contents of UdpPacketInternal would be visible in files where it is not currently visible (loss of scoping).

How? Currently the contents of UDPPacketInternal are accessed through a cast . My suggestion is to make UDPacketInternal an object contained in UDPPacket. It would be accessed as a member. It shouldn't change any scoping and it would eliminate the awkward inheritance.

[ywkaras]

Files that included UDPPacket.h (formerly I_UDPPacket.h) but not UDPPacketInternal.h (formally P_UDPPacket.h) could not access InternalData.

[cmcfarlen]

Based on @maskit 's comment, should we leave this PR as is or add the internal class back or just close this for later?

[ywkaras]

Brian and I don't agree on exactly what the issues are. You and Masakatsu think it's good as-is, so you win by a plurality.