Hi Nilay, On Tue, Apr 14, 2015 at 4:33 PM, Nilay Vaish <[email protected]> wrote:
> On Tue, 14 Apr 2015, Joel Hestness wrote: > > Hi Nilay, >> >> So, I have a couple issues with this: >> 1) You didn't raise this opinion in the review itself, but instead made >> the decision without seeking consensus. >> > > The patch itself is not at variance with what I might have done to address > the issue. Since no one else currently commits patches meant for ruby, I > tend to decide on my own what should be done. Along with this patch, I > committed three other patches that were not discussed at all. Again, in > all such cases, I do what I feel should be done. I understand the desire to shuttle patches through the review process with a slim process. However, the executive decision on this patch was out of line. The gem5 commit access page (http://gem5.org/Commit_Access) specifically states: > > - Your reviews may request some changes; if so, make those and (unless > the changes are trivial) re-post the modified patch for review, updating > the existing review as described above. > > and, at the end of the page: > A patch can be committed after it has been on the reviewboard for 10 days > if no one has commented on it or asked for you to wait. 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. 2) Duplicating a global pointer as members within other objects does not >> change the way that the pointers need to be handled when parallelizing >> Ruby. If there is a shared data member within the RubySystem that needs to >> be locked to ensure consistency, you'll need lock that variable no matter >> how other objects access the RubySystem. Further, the RubySystem global >> pointer is set exactly once in the RubySystem constructor and never >> allowed >> to be changed again. Therefore, there should be no worries about races on >> the global pointer itself, AND distributing RubySystem pointers into other >> objects is more likely to generate potential races if there is ever a need >> to update those pointers. Thus, I strongly disagree with your opinion on >> distributing pointers into the AbstractControllers. >> >> Am I missing something here? >> >> > You can use g_system_ptr, or you can make a local copy of the pointer. To > me both ways are equally fine, neither seems better. It is just that I > prefer not using global variables, so I probably would have gone the same > way as the patch does. 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. 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
