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

[David Blaikie via llvm-commits]

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
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160129/38c5684e/attachment.html>