On Tue, Jun 10, 2008 at 9:33 AM, nathan binkert <[EMAIL PROTECTED]> wrote: >> Thanks for the patches. Just a few comments/questions: >> >> - Does the indirection of the debug info in Event really save >> anything? As long as TRACING_ON is enabled, you're paying an extra >> pointer of storage plus an extra dynamic allocation on every event, >> and it's not clear to me just how much you're gaining. > Yeah, that's true. I was actually planning on replacing TRACING_ON > with EVENTQ_DEBUG. Since it's generally not that useful. > >> - Does anybody actually use the when_scheduled and when_created >> fields? I don't think I ever have. Steve Raasch put them in, I >> believe. If you're concerned about the space overhead I'd just whack >> those two fields entirely, and then leave the instanceId inline (still >> ifdefd with TRACING_ON). > They're useful for debugging. Maybe I'll just put all three inline, > but add the EVENTQ_DEBUG around the other two.
Sounds good to me. > >> - Does it really matter that events with the same tick and priority >> are FIFO vs LIFO? Seems like it would be just as easy to leave it as >> LIFO, which at least wouldn't change the stats. Not saying that's a >> huge reason, but I'm curious what you're motivation is for FIFO that >> trumps that. > I guess it's easy to go either way. I did FIFO because that was > natural. Only after I compared stats and saw that they were different > did I realize that the old queue was LIFO. I never changed it. I > could flip it around to LIFO pretty easily if you think it would be > better to just not change stats. I guess another benefit of LIFO is > that nobody expects to depend on LIFO whereas someone might depend on > FIFO. Since we want to multithread the eventq, you can't really > depend on either. I don't have a strong preference, but if FIFO was a totally random choice, then I don't see a reason not to leave it at LIFO for the reasons you list. > >> - Can you find better names than 'one' and 'two' for those lists? >> Maybe 'nextBin' and 'nextInThisBin'... those aren't great, but at >> least they're descriptive. > The reason I chose one and two was because I had plans for eventually > allowing more than one datastructure. The only time that one and two > matter is if you're actually hacking on the queue and when you're > doing that, you probably understand. Things are commented pretty > well. That said, I can be convinced if you can come up with good > (short) names. I don't think adding a field named 'three' will make it more descriptive :-). And saying "if you're actually hacking on the queue [...] you probably understand" seems like a generic explanation that could fit any obscure coding practice. I had enough trouble in insert_before() remembering whether 'event' was the one I was inserting and 'curr' was the existing event or the other way around. Seriously, 'nextBin' and 'nextInThisBin' are an improvement They're only too long if they make it hard to keep reasonable lines under 80 characters (which I don't expect will be a problem). You only have to type them once (as an argument to 'query-replace'). Almost any other pair of identifiers that had more than one bit of semantic information in them would be an improvement as well... how about 'mainListNext' and 'binListNext'? I'd prefer that they both have "[Nn]ext" in the names since they are linked list pointers. Steve _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
