On Wed, Jul 21, 2021 at 8:26 AM Giacomo Travaglini < [email protected]> wrote:
> Thanks Gabe, > > The performance benefit is difficult to measure as the macro is not > currently used; it could provide negligible or no improvement at all > depending on the compiler and on where it could be used. > So the question is not whether it makes gem5 faster now, it is more about > giving our users the chance to write *potentially* faster code. > I'm not convinced by this because of two things. First, this is saying that there may be a hypothetical example in the future where we *could* see a benefit from this. I don't think just because something *could* be hypothetically useful in the future that that's a good enough reason to keep it. Second, while the performance benefit from this code, even if it wasn't guarding logging, would likely be very small in an absolute sense if anything, it is *also* immediately preceding code which will write to a log file, to the console, etc, (since the presumption is this code *will* output if we get to it), and that is a very high latency sort of operation. That means that we're really talking about a very small or possibly non-existent performance improvement, and stacking it on top of a very slow operation that will completely overwhelm that difference. It's like taking the radio out of a dump truck to save weight, maybe useful in a race car, not useful in these circumstances. > > As you were saying the debug::Flag check could be removed by a later > change. > I'd argue though that a user should be able to catch the unexpected > prints. In some cases, those prints would be even useful, signalling some > debug only logic > has slipped within the non-tracing code. If we take human error into > consideration I am more worried about the dangers of silently executing > debug only > code even when no debug flag is specified. > > To provide a concrete example from the before mentioned patch [1], it > means executing the printQueue method without printing anything. > This won't happen if we use a DPRINTF_UNCONDITIONAL > Well, the problem is that it *will* happen, it's just that the user will have some evidence that it's happening. It would still be up to them to notice and do something about it. Also if they *do* need to call that function (not necessarily the one in your example) for some reason which doesn't have to do with debugging, then they have to either put up with the log output, or make changes to the underlying code that they then need to carry forward as a patch, or try to upstream, or... If it just does what it's supposed to do, they don't have to mess with it and it will just work. It's very reasonable to try to enforce expectations for a particular piece of code like you're saying though. I think in that case you could use an assert or other automatic mechanism which would more clearly and definitively blow up when something was wrong, rather than something that would just behave strangely or suspiciously, with the hope that someone investigates it to find the problem. > > Please let me know your thoughts, > > Kind Regards > > Giacomo > > [1]: > https://gem5-review.googlesource.com/c/public/gem5/+/47199/7/src/mem/cache/prefetch/queued.cc > > > > -----Original Message----- > > From: Gabe Black via gem5-dev <[email protected]> > > Sent: 15 July 2021 23:30 > > To: gem5 Developer List <[email protected]> > > Cc: Gabe Black <[email protected]> > > Subject: [gem5-dev] Re: DPRINTF_UNCONDITIONAL deprecation > > > > Meant to reply to the list. > > > > On Thu, Jul 15, 2021 at 3:29 PM Gabe Black <[email protected] > > <mailto:[email protected]> > wrote: > > > > > > Hi Giacomo. The DPRINTF_UNCONDITIONAL macro was used in one > > place, and that I converted that place to a normal DPRINTF. Please see > here > > for explanation for that change, and justification for this type of > change in > > general: > > > > commit 07074f40e5a7e6000262a5d37f0be836f6ac92a9 > > Author: Gabe Black <[email protected] > > <mailto:[email protected]> > > > Date: Thu Apr 29 20:09:32 2021 -0700 > > > > There are several problems with DPRINTF_UNCONDITIONAL. First, > > while macros don't take any run time overhead because they just turn into > > c++, and of course you could have just typed that c++ to begin with, > they do > > have overhead because they make the code more complex and fragile, and > > obfuscate what's actually going on. This isn't cost incurred (directly) > by the > > computer, but by the people who work on the code. This may eventually > > translate into cost to the computer, because when code is harder to work > > with, it's less likely to be completely correct or optimized. > > > > Second, the compiler is much better at figuring out and keeping > track > > of what code is reachable each time you build. Code paths change, new > > callers are added, etc, and what may have been already guarded by a debug > > variable may not any more. Also, the compiler can already merge together > or > > drop checks it knows are unnecessary, so I think we may frequently be > > dropping something the compiler is already going to drop for us. I think > > DPRINTF_UNCONDITIONAL may be a bit of premature optimization without > > measurement to say that it's really necessary or even useful. > > > > I acknowledge that the lack of performance benefit is also an > > assumption on my part though, and please let me know if you can measure a > > difference in the scenarios you're thinking of. I still think the > complexity and > > fragility argument apply, but if the difference is substantial it might > be still be > > worth it. > > > > Gabe > > > > > > > > On Thu, Jul 15, 2021 at 6:08 AM Giacomo Travaglini > > <[email protected] <mailto:[email protected]> > > > wrote: > > > > > > Hi Gabe, > > > > The DPRINTF_UNCONDITIONAL macro has been deprecated > > in develop [1] and the deprecation is going to be official in v21.1. > > As far as I understood, the reason why it has been > > deprecated is because it was not used in gem5. > > > > I am proposing to remove the deprecation. IMO that macro is > > useful when the Debug flag has already been checked outside of the > > Debug print. We recently merged a patch [2] that would have > > benefitted from the use of DPRINTF_UNCONDITIONAL. I am quite confident > > I can find some other parts of gem5 where the > > DPRINTF_UNCONDITIONAL would have been a better printing choice. > > I believe speed should still be a prime concern even if we > are > > tracing, to make the debugging process faster and smoother for our users, > > especially > > As it comes with no cost (the definition of a macro) > > > > Let me know what you think about this, and if you don't > have > > any objection I will push a revert of the patch you posted, and I will > convert > > some DPRINT into a DPRINTF_UNCONDITIONAL > > > > Kind Regards > > > > Giacomo > > > > [1]: https://gem5- > > review.googlesource.com/c/public/gem5/+/44987 > > [2]: https://gem5- > > review.googlesource.com/c/public/gem5/+/47199/7/src/mem/cache/prefet > > ch/queued.cc#134 > > IMPORTANT NOTICE: The contents of this email and any > > attachments are confidential and may also be privileged. If you are not > the > > intended recipient, please notify the sender immediately and do not > disclose > > the contents to any other person, use it for any purpose, or store or > copy the > > information in any medium. Thank you. > > > > IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy the > information in any medium. Thank you. >
_______________________________________________ gem5-dev mailing list -- [email protected] To unsubscribe send an email to [email protected] %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
