The issue isn't overhead necessarily, although that's always a good thing to consider. There are a lot of general (and well founded) criticisms of how C++ does multiple inheritance from a language perspective, and, for instance, especially tricky and hard to diagnose bugs can crop up when pybind11 (which I'm sure you've seen we use pretty heavily) doesn't have quite the right idea for how classes inherit from other classes, extra bases that get mixed in in the middle somewhere you can't see or didn't notice, etc.
That said, I don't think making multiple inheritance locally *possible* is a bad thing if there isn't performance overhead in sensitive areas (likely not? Still good to be sure) and it doesn't create correctness traps or make writing code which ignores multiple inheritance more difficult/complicated. I do think I've seen some places where multiple inheritance could make things cleaner, for instance by getting rid of the proxy thread context class which is just there (as far as I can tell) to inject an interface into a linearized inheritance hierarchy fot the SimpleThread. Importantly, I think that's just an interface type class which doesn't have a lot of the problems full on multiple inheritance introduces. There are already places where we're using the interface style multiple inheritance, for instance with Drainable and Serializable and the SimObject class. Gabe On Fri, Apr 12, 2019 at 3:04 PM Gambord, Ryan <[email protected]> wrote: > Jason, > > Thank you for taking initiative on this. I can see the point of view that > some of us have regarding historic design decisions. Personally, I think > that if the overhead is not an issue, then it makes sense to add this > feature (which requires relatively minor changes to the codebase) even if > few people want to use it -- what's the harm in having more options? The > need is not super strong on my end right now -- I just came across the note > about it being unsupported, and decided that building in multi-inheritance > would be a good way to get my feet wet with gem5. In working on it, I did > come across a few other areas of the project where it seemed to me that the > authors would have liked to use multi-inheritance, and ended up having to > work around the limitation, but I don't remember off the top of my head. > Maybe other mailing list members can see a value for themselves and want to > chime in? I think the biggest use is in copying entire class hierarchies > and extending them, like I did with the garnet/simple/basic links. > > Ryan Gambord > <[email protected]> > > > > On Wed, Apr 3, 2019 at 9:35 AM Jason Lowe-Power <[email protected]> > wrote: > >> Hi all, >> >> I'm moving this discussion off of gerrit an onto email for now since >> we've derailed from talking about the merits of this patch :). See >> https://gem5-review.googlesource.com/c/public/gem5/+/17609 for the rest >> of the conversation. >> >> @Ryan: First of all, I'd like to apologize for dropping the ball. You >> contacted me a while ago about these patches and I didn't find the time to >> respond. Sorry about that :). It's a deceptively complex change. >> >> I would like to understand better the broader need for multiple >> inheritance. I can see how it helps in the Ruby network example. However, I >> am concerned that there isn't a broader need for it. This is a large >> conceptual change which goes against gem5's historic design decisions (as >> mentioned by Gabe). So, we need to have a very strong reason for making >> this change. >> >> Though, if there is a good reason, we should do it! If the reason for not >> doing it originally was the overhead of virtual functions and compilers are >> now better, then maybe it's time to revisit this design decision. >> >> As far as the Ruby network goes... This code was not originally part of >> m5 and didn't make the same design decisions. I think this is probably why >> you're having problems shoehorning new things into Garnet. I think we >> should look at updating just the Garnet/Ruby network code to be more in >> line with the rest of gem5 before we jump to changing gem5 for Garnet/Ruby, >> if that makes sense. >> >> Ryan, maybe you could prepare a short document describing the >> motivation/need for the changes so we can better understand the background >> (e.g., see what Gabe did for the port updates recently). >> >> Cheers, >> Jason >> >> On Tue, Apr 2, 2019 at 4:30 PM Gambord, Ryan <[email protected]> >> wrote: >> >>> That's an understandable decision. I started working on gem5 recently, >>> so I'm not familiar with the history of the project (nor gerrit, etc) -- >>> apologies for stepping on any toes. I can move the change to wip or abandon >>> if it that's appropriate; I'm not really sure what the generally accepted >>> thing to do is here. >>> >>> I am using multi-inheritance for a new ruby network I am working on. I >>> immediately ran into the below quoted part of SimObject.py, which motivated >>> me to fix multidict and enable support for multiple inheritance in the core >>> files. >>> >>>> 485 <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#485> >>>> # We don't support multiple inheritance of sim objects. If you want486 >>>> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#486> # >>>> to, you must fix multidict to deal with it properly. Non sim-objects487 >>>> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#487> # >>>> are ok, though488 >>>> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#488> >>>> bTotal <http://grok.gem5.org/source/s?defs=bTotal&project=gem5> = 0489 >>>> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#489> >>>> *for* c *in* bases >>>> <http://grok.gem5.org/source/s?defs=bases&project=gem5>:490 >>>> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#490> >>>> *if* isinstance >>>> <http://grok.gem5.org/source/s?defs=isinstance&project=gem5>(c, >>>> MetaSimObject >>>> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#MetaSimObject>):491 >>>> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#491> >>>> bTotal <http://grok.gem5.org/source/s?defs=bTotal&project=gem5> += >>>> 1492 <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#492> >>>> *if* bTotal <http://grok.gem5.org/source/s?defs=bTotal&project=gem5> >>>> > 1:493 <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#493> >>>> *raise* TypeError >>>> <http://grok.gem5.org/source/s?defs=TypeError&project=gem5>(494 >>>> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#494> >>>> "SimObjects do not support multiple inheritance") >>>> >>>> >>> As I have implemented it, this change solely adds support for >>> multiple-inheritance to gem5. SimObjects are not required to implement it >>> if the authors choose not to. I am merely providing the option to those who >>> wish to use it. Unfortunately, to resolve potential ambiguous references in >>> Params when multiple-inheritance is actually used, and because all Params >>> are generated at build time, I did end up having to force virtual >>> inheritance for those. This change simply requires that recasts of >>> SimObject::_params be done dynamically, and that all Params be fully >>> defined rather than forward declared, since each of the params() {} methods >>> cast a virtual base to a derived class. >>> >>> Using virtual inheritance in SimObjects is also simple and painless. It >>> requires only three small changes to the constructor: >>> 1. We add a default argument so that the constructor can be implicitly >>> called by derived classes that inherit virtually from it. >>> 2. We explicitly call the base SimObject(p) instead of the immediate >>> parent class constructor; this is only called by the most derived class in >>> the hierarchy, and causes SimObject::_params to be defined. >>> 3. We then add "p = params();" as the first line of the constructor; >>> this way, if constructor is being called implicitly (as a virtual base >>> class) by a derived class, it picks up the proper _params pointer to use >>> for itself. >>> >>> Here's what the constructor ends up looking like: >>> >>> Before: >>> MySimObject(const Params *p) : MySimObjectParent(p) { ... } >>> After: >>> MySimObject(const Params *p = nullptr) : SimObject(p) { p = >>> params(); ... } >>> >>> >>> There is an alternative to this, which is to use templates where each >>> derived SimObject takes a Params typedef as a template parameter, and >>> passes it up the chain to the base SimObject so it can initialize the >>> SimObject::_params variable with the proper type, which would then mean >>> that base class params() methods would (automatically) upcast rather than >>> downcast as we do now. There are, of course, other options along similar >>> lines, to consider. I did explore this option a bit, but it turned out to >>> be 1. very invasive, requiring updates to a lot of code 2. not really any >>> cleaner or clearer in the end. >>> >>> I have been using this patch locally for several weeks now with no >>> issues and have not encountered any overheads to worry about. For an >>> example of where it makes much more sense to me to use multiple-inheritance >>> is where Simple and Garnet2.0 network links were implemented. The authors >>> ended up adding garnet and simple network specific parameters to the >>> basicLink classes because they couldn't produce the kind of inheritance >>> necessary to extend an entire hierarchy. I've attached a diagram describing >>> the change. Although, in this instance, the code muckiness is relatively >>> minor, I have noticed similar coding practices in various parts of gem5 >>> codebase, where I would assume the authors would have used >>> multiple-inheritance if it were available. >>> >>> Ryan Gambord >>> <[email protected]> >>> >>> >>> >>> On Tue, Apr 2, 2019 at 2:51 PM Gabe Black (Gerrit) < >>> [email protected]> wrote: >>> >>>> To take a step back, not having multiple inheritance was a design >>>> decision that was made a long time ago, I think specifically to avoid all >>>> the craziness with virtual inheritance and all the challenges that come >>>> along with it. Multiple inheritance is allowed when the additional classes >>>> are just interfaces which the class implements which avoid much of the >>>> complexity. I couldn't find any place that was written down unfortunately. >>>> >>>> If we were to allow general multiple inheritance and accept the >>>> overhead of virtual inheritance, etc., then that would also have to be a >>>> design decision with conversation on the mailing list, buy in (or at least >>>> a lack of objection) from everybody, and then an implementation, but here >>>> we're skipping ahead to that last step. >>>> >>>> Personally, I think disallowing multiple/virtual inheritance was a good >>>> decision, and there would need to be a pretty compelling and pressing need >>>> to start allowing it. I don't assert that no such reason exists, but I >>>> haven't seen it yet. >>>> >>>> View Change >>>> <https://gem5-review.googlesource.com/c/public/gem5/+/17609> >>>> >>>> >>>> To view, visit change 17609 >>>> <https://gem5-review.googlesource.com/c/public/gem5/+/17609>. To >>>> unsubscribe, or for help writing mail filters, visit settings >>>> <https://gem5-review.googlesource.com/settings>. >>>> Gerrit-Project: public/gem5 >>>> Gerrit-Branch: master >>>> Gerrit-Change-Id: Ic3664b37f55d0c0f0de95975190794a266c58350 >>>> Gerrit-Change-Number: 17609 >>>> Gerrit-PatchSet: 2 >>>> Gerrit-Owner: Ryan Gambord <[email protected]> >>>> Gerrit-Reviewer: Alexandru Duțu <[email protected]> >>>> Gerrit-Reviewer: Andreas Sandberg <[email protected]> >>>> Gerrit-Reviewer: Gabe Black <[email protected]> >>>> Gerrit-Reviewer: Jason Lowe-Power <[email protected]> >>>> Gerrit-Reviewer: Nikos Nikoleris <[email protected]> >>>> Gerrit-Reviewer: Ryan Gambord <[email protected]> >>>> Gerrit-CC: John Alsop <[email protected]> >>>> Gerrit-Comment-Date: Tue, 02 Apr 2019 21:51:48 +0000 >>>> Gerrit-HasComments: No >>>> Gerrit-Has-Labels: No >>>> Gerrit-MessageType: comment >>>> >>> _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
