[PATCH] D16726: [Profiling] Speed up unittests by ~5x

[David Blaikie via llvm-commits]

Ping

On Mon, Feb 1, 2016 at 6:22 PM, Philip Reames <listmail at philipreames.com>
wrote:

>
>
> On 01/29/2016 10:01 AM, David Blaikie via llvm-commits wrote:
>
>
>
> On Fri, Jan 29, 2016 at 9:48 AM, Xinliang David Li < <davidxl at google.com>
> davidxl at google.com> wrote:
>
>> We are talking about a very small cost here. Even before this change,
>> the time of running this test is ~1s.
>>
>
> Sure - but partly I'm arguing on principles here because the tests all add
> up over time. I'd like to make sure we're developing/sustaining good habits
> about what we add to the test/edit cycle of developers on the project.
>
> +1.
>
>
>
>>
>> David
>>
>> On Fri, Jan 29, 2016 at 9:38 AM, David Blaikie < <dblaikie at gmail.com>
>> dblaikie at gmail.com> wrote:
>> >
>> >
>> > On Fri, Jan 29, 2016 at 9:29 AM, Xinliang David Li <davidxl at google.com>
>> > wrote:
>> >>
>> >> It is the testing the related APIs for PGO profile reader.
>> >
>> >
>> > Sure, but I'm asking why it's insufficient to test it just once/with a
>> > smaller input - what value is added by testing it with such a large
>> input
>> > (why not 512? and if that, why not 1 or some otherwise trivially small
>> > input?) or with 10 different levels of padding?
>> >
>> > Unit tests in particular, and our regression suite in general, are
>> generally
>> > designed to be specifically targeted/purposeful tests. Broader input
>> testing
>> > would be left to the test-suite and/or fuzzing tests (especially
>> coverage
>> > based fuzzing which seems likely to provide better value per CPU cycle
>> than
>> > a more manual/repeated approach like this)
>> >
>> >>
>> >>
>> >> David
>> >>
>> >>
>> >>
>> >> On Fri, Jan 29, 2016 at 9:24 AM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>> >> >
>> >> >
>> >> > On Fri, Jan 29, 2016 at 9:15 AM, Xinliang David Li <
>> davidxl at google.com>
>> >> > wrote:
>> >> >>
>> >> >> The iteration is also encoded in the name.
>> >> >
>> >> >
>> >> > That it tests compression? If we're using a standard compression
>> >> > algorithm,
>> >> > I would expect we would just test the compression algorithm
>> directly. I
>> >> > don't think there's any need to test that the compression algorithm
>> >> > works in
>> >> > any particular use case (test that the contents is compressed).
>> >> >
>> >> > The input seems pretty arbitrarily chosen - it's not clear why that
>> >> > provides
>> >> > the right tradeoff between time and coverage, it looks a bit
>> scattershot
>> >> > ("if we put enough stuff through this, perhaps it'll give confidence
>> >> > that it
>> >> > works") - maybe something better suited to ASan Fuzzing rather than a
>> >> > unit
>> >> > test.
>> >> >
>> >> > Blanket testing with broad inputs probably isn't suitable for a unit
>> >> > test.
>> >> >
>> >> > A single test that demonstrates that the contents is smaller
>> compressed
>> >> > (if
>> >> > there's a non-compressed option) might be worthwhile. Though it looks
>> >> > like
>> >> > this test doesn't even test that - I assume I could remove the
>> >> > compression
>> >> > support from this library & still pass the test...
>> >> >
>> >> >>
>> >> >>
>> >> >> David
>> >> >>
>> >> >> On Fri, Jan 29, 2016 at 9:03 AM, David Blaikie <dblaikie at gmail.com>
>> >> >> wrote:
>> >> >> > If we're asking the question - why does this need to be 1024?
>> What's
>> >> >> > the
>> >> >> > specific purpose of the iteration? (why would 1 be too few
>> >> >> > iterations,
>> >> >> > for
>> >> >> > example?)
>> >> >> >
>> >> >> > On Fri, Jan 29, 2016 at 9:01 AM, David Li via llvm-commits
>> >> >> > <llvm-commits at lists.llvm.org> wrote:
>> >> >> >>
>> >> >> >> davidxl accepted this revision.
>> >> >> >> davidxl added a comment.
>> >> >> >> This revision is now accepted and ready to land.
>> >> >> >>
>> >> >> >> lgtm
>> >> >> >>
>> >> >> >>
>> >> >> >> http://reviews.llvm.org/D16726
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >> _______________________________________________
>> >> >> >> llvm-commits mailing list
>> >> >> >> llvm-commits at lists.llvm.org
>> >> >> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>>
>
>
>
> _______________________________________________
> llvm-commits mailing listllvm-commits at lists.llvm.orghttp://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160208/71cdba46/attachment.html>