Fun, two very different contentious issues in one thread! :) First: If an issue is raised in a review, it's the patch author's responsibility to make sure that either that issue is addressed to the satisfaction of the reviewer, or---in the hopefully rare case where it turns out to be impossible for the author and reviewer to agree on a resolution---that there's a broader consensus among developers on the appropriate decision. Issues should never be resolved unilaterally just by committing code.
Second: Within AMD we have an absolute need to run multiple node instances with Ruby, which is why Brandon has been working on it. I don't see how this could be a contentious goal; actually, the inability to have multiple instances of Ruby is a pretty huge ugly wart on gem5's general modularity and composability. I don't know who originally decided to use a bunch of global variables in Ruby, but if you are ever teaching a software engineering class and need an example of why globals should be avoided, we're ready to do a guest lecture on this use case. I'm glad to have a discussion on the list if anyone has any great ideas about how we should enable multiple instantiations of Ruby other than the approach Nilay and Brandon have taken; as Brandon says, it's ugly and painful, but we don't really see any viable alternatives, and (for better or for worse) abandoning the goal is simply not on the table for us. Steve On Wed, Apr 15, 2015 at 9:45 AM, Joel Hestness <[email protected]> wrote: > On Tue, Apr 14, 2015 at 7:26 PM, Nilay Vaish <[email protected]> wrote: > > > On Tue, 14 Apr 2015, Joel Hestness wrote: > > > > Hi Nilay, > >> > >> In this case, I opened an issue on this patch that I wanted to see > fixed, > >> and I even went so far as to propose what I still view as a more > >> appropriate solution. My request wasn't even addressed. This is very > >> different than no one commenting on a review request and you checking it > >> in. > >> > >> I read nearly all Ruby review requests. I rarely comment, either because > >> I'm fine with the concept of the patch, or I don't have the expertise to > >> quickly understand the changes to the code. I can offer to increase my > >> vocalization in reviews, but we have to make sure we use appropriate > >> process when deciding to check in a patch. If an issue has been opened, > it > >> needs to be addressed. > >> > >> I hope we all agree that executive decisions can create contention and > >> that > >> we should avoid them. > >> > > > > I am guessing the author decided not to take any action. I could have > > done what you suggested, but I did not feel the requirement. > > > If the author decides not to take action, it doesn't mean that the > committer can drop the responsibility of resolving review issues. > > > There are two strong reasons not to use local pointers. First, you don't > >> need a local pointer if there is a global one (unless using the global > one > >> results in a major performance penalty, which is unlikely here). It > >> results > >> in unnecessary object instance bloat. Second, it is inconsistent with > >> other > >> uses of the RubySystem, which almost exclusively use g_system_ptr. There > >> are 11 existing Ruby system classes that use g_system_ptr to access the > >> RubySystem, and basically all generated controllers (including the > >> AbstractControllers!) do also. > >> > >> The only way I can be assuaged is to hear a clearer explanation of why > the > >> AbstractControllers *should* have their own pointers in this patch. At > >> this > >> point, I see no reason. > >> > >> > > If you feel the need, just go ahead and revert the patch and / or commit > > one that uses the global pointer. I am not opposed to using the global > > pointer, but most likely would not do so myself. > > > Making proposed changes is the responsibility of either the author or the > committer, not the reviewers. That said, here is my originally proposed > solution: http://reviews.gem5.org/r/2738/ > > > Joel > > > -- > Joel Hestness > PhD Candidate, Computer Architecture > Dept. of Computer Science, University of Wisconsin - Madison > http://pages.cs.wisc.edu/~hestness/ > _______________________________________________ > gem5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/gem5-dev > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
