Skip to content

Commit

Permalink
Improve CKMS performance (postmates#21)
Browse files Browse the repository at this point in the history
* Introduce criterion benchmarks for CKMS

This commit introduces the [criterion](https://github.com/japaric/criterion.rs)
library into quantiles. The intention is to tackle CKMS performance
again.

Signed-off-by: Brian L. Troutwine <[email protected]>

* Fiddle some with the performance characteristics

This commit cuts the size of Entry a by 64 bits, shaving milliseconds
off the 10k benchmark run. We're still waaaay too slow for 65k to
really notice a good difference there.

Signed-off-by: Brian L. Troutwine <[email protected]>

* Finish adding documentation for the night

Time to wrap it up. I'll have to shop this around to folks that
have a better handle on wacky tricks and/or know of a DS of greater
applicability than Vec. I am stuck for sure.

Signed-off-by: Brian L. Troutwine <[email protected]>

* Introduce 'Store'

This commit is inspired by the discussion [here](https://users.rust-lang.org/t/how-can-i-optimize-this-data-structure/14273/7).
I'm not sure yet how well it does, benchmark wise.

Signed-off-by: Brian L. Troutwine <[email protected]>

* Bookmark commit

In this commit we're set to run comparative benchmarks for Vec and
Store. The results were not promising, with Vec mean being 493ms
and Store mean being 546. Better SD and MAD on Vec too.

The computation of 'r' can be stored at the top of each inner store
and we can keep a sorted buffer for insertion. I guess the thing to
do is benchmark Vec against Store, see where we end up.

Signed-off-by: Brian L. Troutwine <[email protected]>

* Go all-in on Store, perform compaction of inner data

This commit moves CKMS fully onto store with the understanding
that the current implementation is pokier than just plain Vec on
account of iteration on insertion. The expectation is that we
can resolve this.

Benchmarking -- not shown -- indicates that Store can do inserts
about twice as fast as Vec _if_ we don't have to seek over the
whole thing before inserting.

Signed-off-by: Brian L. Troutwine <[email protected]>

* WIP commit

This commit goes quite a ways toward making 'stores' a proper
backing store for CKMS. We just need AddAssign going again.

Signed-off-by: Brian L. Troutwine <[email protected]>

* Re-introduce AddAssign to CKMS

Signed-off-by: Brian L. Troutwine <[email protected]>

* Better footing for benchmarks

I've removed sum from the interface of CKMS because if the type
is especially small I don't have a wrapping_add available to me.
Oops. Up to the client to keep track of this, as a result.

Well, maybe if I look into Wrapping?

Anyway! We're now on a good footing to benchmark this thing.

Signed-off-by: Brian L. Troutwine <[email protected]>

* Switch benchmark back to 65k.

Signed-off-by: Brian L. Troutwine <[email protected]>

* Correct benchmarking

I've backed off the use of criterion for now since I couldn't figure
out how to slot it into our existing macro-based benchmark generation.
This _should_ now pass CI.

Signed-off-by: Brian L. Troutwine <[email protected]>

* Introduce comments to store.rs, fuzz tests

This commit adds the last bit of polish to store.rs in terms of
commenting on its function. We also introduce fuzz tests for
inspecting the CKMS implementation. -max_len=160 ought to be in
place for the existing fuzz test.

Signed-off-by: Brian L. Troutwine <[email protected]>
  • Loading branch information
blt authored Dec 11, 2017
1 parent 0c6563c commit 08b4e46
Show file tree
Hide file tree
Showing 8 changed files with 681 additions and 211 deletions.
4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "quantiles"
version = "0.6.3"
version = "0.7.0-pre"
authors = ["Brian L. Troutwine <[email protected]>"]

description = "a collection of approximate quantile algorithms"
Expand All @@ -15,7 +15,7 @@ keywords = ["statistics", "histogram", "quantiles", "percentiles", "approximatio
lto = true

[dev-dependencies]
quickcheck = "0.4"
quickcheck = "0.5"

[dependencies]
serde = { version = "1.0", optional = true }
Expand Down
21 changes: 17 additions & 4 deletions benches/ckms.rs
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
#![feature(test)]

extern crate quantiles;
extern crate test;
extern crate quantiles;

mod ckms {
#[derive(Debug, Clone, Copy)]
pub struct Xorshift {
seed: u64,
}

impl Xorshift {
pub fn new(seed: u64) -> Xorshift {
Xorshift { seed: seed }
}

pub fn next_val(&mut self) -> u32 {
// implementation inspired by
// https://github.com/astocko/xorshift/blob/master/src/splitmix64.rs
use std::num::Wrapping as w;

let mut z = w(self.seed) + w(0x9E37_79B9_7F4A_7C15_u64);
let nxt_seed = z.0;
z = (z ^ (z >> 30)) * w(0xBF58_476D_1CE4_E5B9_u64);
Expand Down Expand Up @@ -94,4 +94,17 @@ mod ckms {
generate_primed_tests!(u32, bench_primed_65535, 65_535);
}

mod f32 {
use super::*;

generate_tests!(f32, bench_insert_100, 100);
generate_tests!(f32, bench_insert_1000, 1000);
generate_tests!(f32, bench_insert_10000, 10_000);
generate_tests!(f32, bench_insert_100000, 100_000);

generate_primed_tests!(f32, bench_primed_100, 100);
generate_primed_tests!(f32, bench_primed_1000, 1000);
generate_primed_tests!(f32, bench_primed_10000, 10_000);
generate_primed_tests!(f32, bench_primed_65535, 65_535);
}
}
4 changes: 4 additions & 0 deletions fuzz/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@

target
corpus
artifacts
24 changes: 24 additions & 0 deletions fuzz/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
[package]
name = "quantiles-fuzz"
version = "0.0.1"
authors = ["Automatically generated"]
publish = false

[package.metadata]
cargo-fuzz = true

[dependencies]
byteorder = "1.2"

[dependencies.quantiles]
path = ".."
[dependencies.libfuzzer-sys]
git = "https://github.com/rust-fuzz/libfuzzer-sys.git"

# Prevent this from interfering with workspaces
[workspace]
members = ["."]

[[bin]]
name = "ckms_u32"
path = "fuzz_targets/ckms_u32.rs"
61 changes: 61 additions & 0 deletions fuzz/fuzz_targets/ckms_u32.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
#![no_main]
#[macro_use] extern crate libfuzzer_sys;
extern crate quantiles;
extern crate byteorder;

use std::io::Cursor;
use byteorder::{BigEndian, ReadBytesExt};

#[derive(Debug, Clone, Copy)]
pub struct Xorshift {
seed: u64,
}

impl Xorshift {
pub fn new(seed: u64) -> Xorshift {
Xorshift { seed: seed }
}

pub fn next_val(&mut self) -> u32 {
// implementation inspired by
// https://github.com/astocko/xorshift/blob/master/src/splitmix64.rs
use std::num::Wrapping as w;

let mut z = w(self.seed) + w(0x9E37_79B9_7F4A_7C15_u64);
let nxt_seed = z.0;
z = (z ^ (z >> 30)) * w(0xBF58_476D_1CE4_E5B9_u64);
z = (z ^ (z >> 27)) * w(0x94D0_49BB_1331_11EB_u64);
self.seed = nxt_seed;
u32::from((z ^ (z >> 31)).0 as u16)
}
}

fuzz_target!(|data: &[u8]| {
let mut cursor = Cursor::new(data);

// unbounded, CKMS will adjust to within bounds
let error: f64 = if let Ok(res) = cursor.read_f64::<BigEndian>() {
res
} else {
return;
};
// bounded 2**24
let upper_bound: u32 = if let Ok(res) = cursor.read_u32::<BigEndian>() {
res % 16_777_216
} else {
return;
};
// unbounded
let seed: u64 = if let Ok(res) = cursor.read_u64::<BigEndian>() {
res
} else {
return;
};

let mut xshft = Xorshift::new(seed);
let mut ckms = quantiles::ckms::CKMS::<u32>::new(error);
for _ in 0..(upper_bound as usize) {
let val = xshft.next_val();
ckms.insert(val);
}
});
32 changes: 32 additions & 0 deletions src/ckms/entry.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
use std::cmp;

#[derive(Debug, Clone)]
#[cfg_attr(feature = "serde_support", derive(Serialize, Deserialize))]
pub struct Entry<T>
where
T: PartialEq,
{
pub g: u32,
pub delta: u32,
pub v: T,
}

// The derivation of PartialEq for Entry is not appropriate. The sole ordering
// value in an Entry is the value 'v'.
impl<T> PartialEq for Entry<T>
where
T: PartialEq,
{
fn eq(&self, other: &Entry<T>) -> bool {
self.v == other.v
}
}

impl<T> PartialOrd for Entry<T>
where
T: PartialOrd,
{
fn partial_cmp(&self, other: &Entry<T>) -> Option<cmp::Ordering> {
self.v.partial_cmp(&other.v)
}
}
Loading

0 comments on commit 08b4e46

Please sign in to comment.