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

[Xinliang David Li via llvm-commits]

On Mon, Feb 8, 2016 at 4:12 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Mon, Feb 8, 2016 at 4:00 PM, Xinliang David Li <davidxl at google.com>
> wrote:
>>
>> Yes, if we talk about all tests in different repos collectively, 0 is
>> probably covered -- but that is still an assumption from the
>> unittest's point of view.
>
>
> Ah, sure - I agree it (along with any other API surface area) should be
> covered by a unit test, even if the codepath is also covered by a higher
> level test of some kind.
>
> But it looks like the 0 padding case is covered by other /unit/ tests in the
> same file.

AFAIK, this is the only unittest that covers 0 padding case.

> This seems like a reasonable case of test case reduction by
> combination (if you have {a, b} x {x, y} to test you can test it in two
> cases {a, x}, {b, y} rather than running 4 tests, since the features are
> independent).

We need to be very careful about reducing {a,b}x{x,y} into {a,x} and
{b,y} though -- it makes explicit assumption about the implementation
-- there might be subtle dependencies there..

David

>
>>
>>
>> David
>>
>> On Mon, Feb 8, 2016 at 3:55 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 3:53 PM, Xinliang David Li <davidxl at google.com>
>> > wrote:
>> >>
>> >> On Mon, Feb 8, 2016 at 3:30 PM, David Blaikie <dblaikie at gmail.com>
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 3:17 PM, Xinliang David Li
>> >> > <davidxl at google.com>
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 3:12 PM, David Blaikie <dblaikie at gmail.com>
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 3:08 PM, Xinliang David Li
>> >> >> > <davidxl at google.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Mon, Feb 1, 2016 at 10:44 AM, David Blaikie
>> >> >> >> <dblaikie at gmail.com>
>> >> >> >> wrote:
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > On Fri, Jan 29, 2016 at 5:58 PM, Xinliang David Li
>> >> >> >> > <davidxl at google.com>
>> >> >> >> > wrote:
>> >> >> >> >>
>> >> >> >> >> To clarify, it is not 128 iterations, but creating a symbol
>> >> >> >> >> table
>> >> >> >> >> with
>> >> >> >> >> 128 entries -- which is a reasonable size.
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > We don't generally test on "realistic" sized inputs in the
>> >> >> >> > regression
>> >> >> >> > suite.
>> >> >> >> > We write targeted tests for functionality. Broad testing is
>> >> >> >> > done
>> >> >> >> > in
>> >> >> >> > the
>> >> >> >> > test-suite and other integration level testing.
>> >> >> >> >
>> >> >> >> >>
>> >> >> >> >> Test coverage wise, it is probably the same as a 3-entry
>> >> >> >> >> symtab.
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > Then let's use a 3-entry symtab.
>> >> >> >> >
>> >> >> >> > (why 3? Because it tests the boundaries (first and last) and
>> >> >> >> > one
>> >> >> >> > "normal"
>> >> >> >> > case of a non-boundary value - while the boundaries probably
>> >> >> >> > aren't
>> >> >> >> > interesting in this algorithm, it's cheap enough to just follow
>> >> >> >> > that
>> >> >> >> > common
>> >> >> >> > practice in test case design)
>> >> >> >>
>> >> >> >> Will update it to 3.
>> >> >> >>
>> >> >> >> >
>> >> >> >> > I'm also curious about the padding parameter - what does it do?
>> >> >> >> > Choose
>> >> >> >> > how
>> >> >> >> > many null characters go between each value? What effect does
>> >> >> >> > that
>> >> >> >> > have/why
>> >> >> >> > is that a tuning parameter? (understanding what it's for can
>> >> >> >> > help
>> >> >> >> > us
>> >> >> >> > choose
>> >> >> >> > appropriate test cases/coverage for that functionality)
>> >> >> >>
>> >> >> >> Internal padding bytes (for alignment to 4 bytes) can be zero to
>> >> >> >> 3.
>> >> >> >
>> >> >> >
>> >> >> > Any idea what's particularly useful to test here? (does it just
>> >> >> > assert
>> >> >> > that
>> >> >> > the parameter is [0,3] ? Or does it have well defined behavior
>> >> >> > (returning an
>> >> >> > error code? doing something else?) outside that range? is any case
>> >> >> > more
>> >> >> > interesting than any other - or just a simple loop for [0,Padding]
>> >> >> > done
>> >> >> > at
>> >> >> > some point in the algorithm? Does anything test that the algorithm
>> >> >> > emitted
>> >> >> > the right padding?)
>> >> >>
>> >> >> It tests that the reader is (flexible) and capable of handing
>> >> >> padding
>> >> >> bytes not produced by the writer.  How many paddings should be
>> >> >> emitted
>> >> >> is not specified. For instance, if some producer forces 8 byte
>> >> >> alignment, it should be handled too.
>> >> >
>> >> >
>> >> > Ah, OK - perhaps we could just test one pseudo-random (if it's really
>> >> > just a
>> >> > "while (null byte)" loop to ignore the padding - I'd probably pick 2
>> >> > bytes
>> >> > of padding, but don't mind any small number) amount of padding to
>> >> > test
>> >> > that
>> >> > the reader ignores it, rather than testing several amounts of
>> >> > padding?
>> >> > Alternatively/in addition, might be good to test these features
>> >> > separately
>> >> > to make triage easier? Rather than combining compression and padding
>> >> > together - unless there's an interesting interaction between the two
>> >> > features in the implementation?
>> >> >
>> >>
>> >> I think 0 is more special here, so I would pick 0 and 1 byte.
>> >
>> >
>> > Is zero bytes of padding not already covered by any other tests? (I
>> > assume
>> > it's covered by most tests as it sounds like it's the common case?)
>> >
>> >>
>> >> > You say "padding bytes not produced by the writer" - does the writer
>> >> > produce
>> >> > zero bytes of padding, or some amount of padding that's just not the
>> >> > same
>> >> > amounts as are being tested here?
>> >>
>> >> The writer can produce 0 or more padding bytes, the assembler and
>> >> linker may or may not pad more. The purpose of the testing is that the
>> >> reader does not depend/care about those behavior.
>> >>
>> >> David
>> >>
>> >> >
>> >> > - David
>> >
>> >
>
>