BurntSushi / quickcheck

Automated property based testing for Rust (with shrinking).
The Unlicense
2.4k stars 149 forks source link

Rewrite `impl Arbitrary for tuples` macros #186

Closed hcpl closed 6 years ago

hcpl commented 6 years ago

This implementation relies less on cloning while shrinking, which is a performance boost, evident from benchmarks:

 name                   before ns/iter  after ns/iter  diff ns/iter   diff %  speedup 
 shrink_f64_1_tuple     607             607                       0    0.00%   x 1.00 
 shrink_f64_2_tuple     1,099           1,019                   -80   -7.28%   x 1.08 
 shrink_f64_3_tuple     1,932           1,657                  -275  -14.23%   x 1.17 
 shrink_f64_4_tuple     2,925           2,448                  -477  -16.31%   x 1.19 
 shrink_f64_5_tuple     4,041           3,368                  -673  -16.65%   x 1.20 
 shrink_f64_6_tuple     5,989           4,790                -1,199  -20.02%   x 1.25 
 shrink_f64_7_tuple     8,524           6,207                -2,317  -27.18%   x 1.37 
 shrink_f64_8_tuple     11,510          7,243                -4,267  -37.07%   x 1.59 
 shrink_i64_1_tuple     461             517                      56   12.15%   x 0.89 
 shrink_i64_2_tuple     896             912                      16    1.79%   x 0.98 
 shrink_i64_3_tuple     1,550           1,597                    47    3.03%   x 0.97 
 shrink_i64_4_tuple     2,658           2,610                   -48   -1.81%   x 1.02 
 shrink_i64_5_tuple     3,878           3,721                  -157   -4.05%   x 1.04 
 shrink_i64_6_tuple     5,163           4,492                  -671  -13.00%   x 1.15 
 shrink_i64_7_tuple     8,168           5,915                -2,253  -27.58%   x 1.38 
 shrink_i64_8_tuple     10,065          7,233                -2,832  -28.14%   x 1.39 
 shrink_string_1_tuple  250,662         258,711               8,049    3.21%   x 0.97 
 shrink_string_2_tuple  381,550         367,213             -14,337   -3.76%   x 1.04 
 shrink_string_3_tuple  866,201         816,432             -49,769   -5.75%   x 1.06 
 shrink_string_4_tuple  1,001,036       984,872             -16,164   -1.61%   x 1.02 
 shrink_string_5_tuple  1,243,482       1,210,580           -32,902   -2.65%   x 1.03 
 shrink_string_6_tuple  1,870,379       1,761,332          -109,047   -5.83%   x 1.06 
 shrink_string_7_tuple  5,214,538       4,331,629          -882,909  -16.93%   x 1.20 
 shrink_string_8_tuple  7,596,625       5,974,834        -1,621,791  -21.35%   x 1.27 
 shrink_u64_1_tuple     302             334                      32   10.60%   x 0.90 
 shrink_u64_2_tuple     833             686                    -147  -17.65%   x 1.21 
 shrink_u64_3_tuple     1,566           1,162                  -404  -25.80%   x 1.35 
 shrink_u64_4_tuple     1,907           1,866                   -41   -2.15%   x 1.02 
 shrink_u64_5_tuple     2,553           2,486                   -67   -2.62%   x 1.03 
 shrink_u64_6_tuple     3,848           3,141                  -707  -18.37%   x 1.23 
 shrink_u64_7_tuple     5,666           4,140                -1,526  -26.93%   x 1.37 
 shrink_u64_8_tuple     7,470           5,489                -1,981  -26.52%   x 1.36 
 shrink_unit_1_tuple    92              85                       -7   -7.61%   x 1.08 
 shrink_unit_2_tuple    203             102                    -101  -49.75%   x 1.99 
 shrink_unit_3_tuple    301             119                    -182  -60.47%   x 2.53 
 shrink_unit_4_tuple    396             142                    -254  -64.14%   x 2.79 
 shrink_unit_5_tuple    472             159                    -313  -66.31%   x 2.97 
 shrink_unit_6_tuple    577             187                    -390  -67.59%   x 3.09 
 shrink_unit_7_tuple    667             196                    -471  -70.61%   x 3.40 
 shrink_unit_8_tuple    765             217                    -548  -71.63%   x 3.53 
 shrink_vec_u8_1_tuple  72,397          84,544               12,147   16.78%   x 0.86 
 shrink_vec_u8_2_tuple  437,995         468,957              30,962    7.07%   x 0.93 
 shrink_vec_u8_3_tuple  1,300,548       1,249,062           -51,486   -3.96%   x 1.04 
 shrink_vec_u8_4_tuple  1,782,403       1,753,314           -29,089   -1.63%   x 1.02 
 shrink_vec_u8_5_tuple  2,266,855       2,224,065           -42,790   -1.89%   x 1.02 
 shrink_vec_u8_6_tuple  3,435,422       3,151,182          -284,240   -8.27%   x 1.09 
 shrink_vec_u8_7_tuple  5,081,438       4,118,840          -962,598  -18.94%   x 1.23 
 shrink_vec_u8_8_tuple  6,095,051       4,692,820        -1,402,231  -23.01%   x 1.30

Also makes macros more readable (I hope so).

hcpl commented 6 years ago

There are some cases with regressions, which are all short tuples:

 shrink_i64_1_tuple     461             517                      56   12.15%   x 0.89 
 shrink_i64_2_tuple     896             912                      16    1.79%   x 0.98 
 shrink_i64_3_tuple     1,550           1,597                    47    3.03%   x 0.97
 shrink_string_1_tuple  250,662         258,711               8,049    3.21%   x 0.97
 shrink_u64_1_tuple     302             334                      32   10.60%   x 0.90
 shrink_vec_u8_1_tuple  72,397          84,544               12,147   16.78%   x 0.86 
 shrink_vec_u8_2_tuple  437,995         468,957              30,962    7.07%   x 0.93

That's due to how the previous implementation was written:

fn shrink(&self)
         -> Box<Iterator<Item=($type_a, $($type_n),*)>> {
    let (ref $var_a, $(ref $var_n),*) = *self;
    let sa = $var_a.shrink().scan(
        ($($var_n.clone(),)*),
        |&mut ($(ref $var_n,)*), $var_a|
            Some(($var_a, $($var_n.clone(),)*))
    );
    let srest = ($($var_n.clone(),)*).shrink()
        .scan($var_a.clone(), |$var_a, ($($var_n,)*)|
            Some(($var_a.clone(), $($var_n,)*))
        );
    Box::new(sa.chain(srest))
}

For 1-tuple this expands to

fn shrink(&self)
        -> Box<Iterator<Item=(A,)>> {
    let (ref a,) = *self;
    let sa = a.shrink().scan(
        (),
        |&mut (), a|
            Some((a,))
    );
    let srest = ().shrink()
        .scan(a.clone(), |a, ()|
            Some((a.clone(),))
        );
    Box::new(sa.chain(srest))
}

... this translates to:

fn shrink(&self) -> Box<Iterator<Item=(A,)>> {
    let sa = self.0.shrink();
    let srest = empty_shrinker();
    Box::new(sa.chain(srest))
}

... which doesn't even have any cloning in the first place.

For comparison, the implementation proposed in this PR works like this:

fn shrink(&self) -> Box<Iterator<Item = (A,)>> {
    let cloned_for_0 = self.clone();
    Box::new(
        ::std::iter::empty()
            .chain(self.0.shrink().map(move |shr_value| {
                let mut result = cloned_for_0.clone();
                result.0 = shr_value;
                result
            })),
    )
}

So the question is: should I special-case for 1-tuples (and maybe 2- and 3- as well) or it's not worth the efforts and this will cause maintenance burden.

Also if this library focuses on performance for shorter cases (which I guess are much more often needed, because functions tend to have few arguments and they rely on tuples) and readability is less a concern, feel free to close this PR.

BurntSushi commented 6 years ago

Thanks! The performance result of this isn't too concerning to me. This library really wasn't implemented with performance in mind, and cloning is used with reckless abandon. This can definitely be a pain point in some cases, but I more often hit cases where testing the property itself takes a bit of time and causes the overall test to be a bit lengthy.

With that said, it seems like this PR simplifies the code, which is the win I care about, so I'll take it. :-)