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

[Philip Reames via llvm-commits]

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 
> <mailto: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
>     <mailto:dblaikie at gmail.com>> wrote:
>     >
>     >
>     > On Fri, Jan 29, 2016 at 9:29 AM, Xinliang David Li
>     <davidxl at google.com <mailto: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 <mailto:dblaikie at gmail.com>> wrote:
>     >> >
>     >> >
>     >> > On Fri, Jan 29, 2016 at 9:15 AM, Xinliang David Li
>     <davidxl at google.com <mailto: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 <mailto: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
>     <mailto: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
>     <mailto:llvm-commits at lists.llvm.org>
>     >> >> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>     >> >> >
>     >> >> >
>     >> >
>     >> >
>     >
>     >
>
>
>
>
> _______________________________________________
> 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/20160201/c4624f89/attachment.html>