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

[Xinliang David Li via llvm-commits]

We are talking about a very small cost here. Even before this change,
the time of running this test is ~1s.

David

On Fri, Jan 29, 2016 at 9:38 AM, David Blaikie <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
>> >> >
>> >> >
>> >
>> >
>
>