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

[David Blaikie via llvm-commits]

On Fri, Jan 29, 2016 at 9:48 AM, Xinliang David Li <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.


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