[llvm-commits] [PATCH][LNT] Add support for selection of timing statistic (user or real time)

[David Blaikie]

On Wed, Aug 15, 2012 at 11:26 AM, Daniel Dunbar <daniel at zuster.org> wrote:
> On Wed, Aug 15, 2012 at 10:57 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> On Wed, Aug 15, 2012 at 10:34 AM, Daniel Dunbar <daniel at zuster.org> wrote:
>>> Hi David,
>>>
>>> Sorry for review lag, I've tried to fix my filters so I see this stuff sooner.
>>
>> Thanks
>>
>>> The LNT side of the patch looks fine, but I think the test-suite side
>>> would be substantially simplified if you just added an option to
>>> timeit-target to report the real time instead of the user time as the
>>> "program time". This would avoid needing most of the other changes.
>>
>> It would reduce the number of changes, but I think it would
>> complicate, rather than simplify, the test-suite (which is already way
>> more complicated than it should be due to similar "smallest change"
>> rather than "simplest post-patch state" tradeoffs, it seems to me).
>> That option did cross my mind - honestly at this point I forget
>> whether I tried that or how far I got down the path before I worked on
>> this version.
>>
>> Part of the point was to provide more information up to reports & then
>> let reports decide how to format that data - or even to report
>> multiple statistics to LNT & then we could investigate combinations or
>> selections of data points we might find interesting.
>
> Ok. In that case, I'm fine with the patch.

Thanks.

Committed the test-suite portion as r162240.
Committed the LNT portion as r162241.

>
>>
>>> Also, I can't imagine how your patch actually works given that it
>>> deletes the --summary option, but the test suite still uses it?
>>
>> That one's easy: 20 lines up there's exactly the same 9 lines of code.
>> This is just removing some weird copy-paste redundancy, so far as I
>> can tell.
>
> Oops, thanks then!

NP - I probably should've pulled that out & committed it separately
instead of jumbling up this code review with it anyway.

Committed separately as r162237.

Thanks,
- David