Merge pull request #1050 from rylev/refactor-graph

rylev · web-flow · commit 265aa9447e37 · 2021-10-04T22:22:05.000+02:00
Refactorings: graph =&gt; graphs
diff --git a/site/src/api.rs b/site/src/api.rs
@@ -70,7 +70,7 @@ pub mod graph {
         // y-values
         pub points: Vec<f32>,
         // The index of interpolated coordinates
-        pub is_interpolated: HashSet<u16>,
+        pub interpolated_indices: HashSet<u16>,
     }
 
     #[derive(Debug, PartialEq, Clone, Serialize)]
diff --git a/site/src/average.rs b/site/src/average.rs
@@ -110,7 +110,7 @@ mod tests {
     fn test_interpolation_average() {
         // Test that averaging works with interpolation.
         use crate::db::Point;
-        use crate::interpolate::{Interpolate, Interpolated};
+        use crate::interpolate::{Interpolate, IsInterpolated};
 
         let v = vec![
             vec![("a", Some(0.0)), ("b", Some(200.0))],
@@ -125,11 +125,11 @@ mod tests {
         let mut average = average(iterators);
 
         let a = average.next().unwrap();
-        assert_eq!(a, (("a", Some(50.0)), Interpolated::No));
+        assert_eq!(a, (("a", Some(50.0)), IsInterpolated::No));
         assert_eq!(a.interpolated(), false);
 
         let b = average.next().unwrap();
-        assert_eq!(b, (("b", Some(150.0)), Interpolated::Yes));
+        assert_eq!(b, (("b", Some(150.0)), IsInterpolated::Yes));
         assert_eq!(b.interpolated(), true);
 
         assert!(average.next().is_none());
diff --git a/site/src/interpolate.rs b/site/src/interpolate.rs
@@ -5,22 +5,27 @@
 //! mostly want to avoid dropping or improving summary performance when data
 //! points are missing as that's misleading and noisy, and this works well for
 //! that.
+//!
+//! As an example:
+//! Given a series with some missing data `[1, 2, ?, 4]`,
+//! this iterator yields `[1, 2, 2, 4]`.
 
 use crate::db::Point;
 
+/// Whether a point has been interpolated or not
 #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
-pub enum Interpolated {
+pub enum IsInterpolated {
     No,
     Yes,
 }
 
-impl Interpolated {
-    pub fn is_interpolated(self) -> bool {
-        self == Interpolated::Yes
+impl IsInterpolated {
+    pub fn as_bool(self) -> bool {
+        self == IsInterpolated::Yes
     }
 }
 
-impl<P> Point for (P, Interpolated)
+impl<P> Point for (P, IsInterpolated)
 where
     P: Point,
 {
@@ -39,21 +44,23 @@ where
         self.0.set_value(value)
     }
     fn interpolated(&self) -> bool {
-        self.1.is_interpolated()
+        self.1.as_bool()
     }
     fn set_interpolated(&mut self) {
-        self.1 = Interpolated::Yes;
+        self.1 = IsInterpolated::Yes;
     }
 }
 
+/// The interpolated iterator
 pub struct Interpolate<I>
 where
     I: Iterator,
 {
+    /// The base iterator we're interpolating
     iterator: I,
+    /// The last seen point which will be used for interpolation
     last_seen: Option<f64>,
-
-    // When we need to seek forward at the start, we store things in here.
+    /// When we need to seek forward at the start, we store things in here.
     consumed: Vec<I::Item>,
 }
 
@@ -76,15 +83,15 @@ where
     I: Iterator,
     I::Item: Point,
 {
-    type Item = (I::Item, Interpolated);
+    type Item = (I::Item, IsInterpolated);
 
     fn next(&mut self) -> Option<Self::Item> {
         if let Some(mut item) = self.consumed.pop() {
             item.set_value(self.last_seen.unwrap());
             let interpolation = if self.consumed.is_empty() {
-                Interpolated::No
+                IsInterpolated::No
             } else {
-                Interpolated::Yes
+                IsInterpolated::Yes
             };
             return Some((item, interpolation));
         }
@@ -94,12 +101,12 @@ where
         match item.value() {
             Some(pt) => {
                 self.last_seen = Some(pt);
-                return Some((item, Interpolated::No));
+                return Some((item, IsInterpolated::No));
             }
             None => {
                 if let Some(last) = self.last_seen {
                     item.set_value(last);
-                    return Some((item, Interpolated::Yes));
+                    return Some((item, IsInterpolated::Yes));
                 }
 
                 self.consumed.push(item);
@@ -122,7 +129,7 @@ where
 
                                     let mut item = self.consumed.pop().unwrap();
                                     item.set_value(self.last_seen.unwrap());
-                                    return Some((item, Interpolated::Yes));
+                                    return Some((item, IsInterpolated::Yes));
                                 }
                             }
                         }
@@ -139,15 +146,15 @@ where
 
 #[cfg(test)]
 mod tests {
-    use super::{Interpolate, Interpolated};
+    use super::{Interpolate, IsInterpolated};
 
     #[test]
     fn test_no_interpolation() {
         let v = vec![("a", 1.0), ("b", 2.0)];
         let mut iter = Interpolate::new(v.into_iter());
 
-        assert_eq!(iter.next().unwrap(), (("a", 1.0), Interpolated::No));
-        assert_eq!(iter.next().unwrap(), (("b", 2.0), Interpolated::No));
+        assert_eq!(iter.next().unwrap(), (("a", 1.0), IsInterpolated::No));
+        assert_eq!(iter.next().unwrap(), (("b", 2.0), IsInterpolated::No));
         assert!(iter.next().is_none());
     }
 
@@ -156,10 +163,16 @@ mod tests {
         let v = vec![("a", None), ("b", None), ("c", Some(3.0)), ("d", Some(4.0))];
         let mut iter = Interpolate::new(v.into_iter());
 
-        assert_eq!(iter.next().unwrap(), (("a", Some(3.0)), Interpolated::Yes));
-        assert_eq!(iter.next().unwrap(), (("b", Some(3.0)), Interpolated::Yes));
-        assert_eq!(iter.next().unwrap(), (("c", Some(3.0)), Interpolated::No));
-        assert_eq!(iter.next().unwrap(), (("d", Some(4.0)), Interpolated::No));
+        assert_eq!(
+            iter.next().unwrap(),
+            (("a", Some(3.0)), IsInterpolated::Yes)
+        );
+        assert_eq!(
+            iter.next().unwrap(),
+            (("b", Some(3.0)), IsInterpolated::Yes)
+        );
+        assert_eq!(iter.next().unwrap(), (("c", Some(3.0)), IsInterpolated::No));
+        assert_eq!(iter.next().unwrap(), (("d", Some(4.0)), IsInterpolated::No));
         assert!(iter.next().is_none());
     }
 
@@ -175,12 +188,18 @@ mod tests {
         ];
         let mut iter = Interpolate::new(v.into_iter());
 
-        assert_eq!(iter.next().unwrap(), (("a", Some(1.0)), Interpolated::No));
-        assert_eq!(iter.next().unwrap(), (("b", Some(2.0)), Interpolated::No));
-        assert_eq!(iter.next().unwrap(), (("c", Some(2.0)), Interpolated::Yes));
-        assert_eq!(iter.next().unwrap(), (("d", Some(2.0)), Interpolated::Yes));
-        assert_eq!(iter.next().unwrap(), (("e", Some(5.0)), Interpolated::No));
-        assert_eq!(iter.next().unwrap(), (("f", Some(6.0)), Interpolated::No));
+        assert_eq!(iter.next().unwrap(), (("a", Some(1.0)), IsInterpolated::No));
+        assert_eq!(iter.next().unwrap(), (("b", Some(2.0)), IsInterpolated::No));
+        assert_eq!(
+            iter.next().unwrap(),
+            (("c", Some(2.0)), IsInterpolated::Yes)
+        );
+        assert_eq!(
+            iter.next().unwrap(),
+            (("d", Some(2.0)), IsInterpolated::Yes)
+        );
+        assert_eq!(iter.next().unwrap(), (("e", Some(5.0)), IsInterpolated::No));
+        assert_eq!(iter.next().unwrap(), (("f", Some(6.0)), IsInterpolated::No));
         assert!(iter.next().is_none());
     }
 
@@ -189,10 +208,16 @@ mod tests {
         let v = vec![("a", Some(1.0)), ("b", Some(2.0)), ("c", None), ("d", None)];
         let mut iter = Interpolate::new(v.into_iter());
 
-        assert_eq!(iter.next().unwrap(), (("a", Some(1.0)), Interpolated::No));
-        assert_eq!(iter.next().unwrap(), (("b", Some(2.0)), Interpolated::No));
-        assert_eq!(iter.next().unwrap(), (("c", Some(2.0)), Interpolated::Yes));
-        assert_eq!(iter.next().unwrap(), (("d", Some(2.0)), Interpolated::Yes));
+        assert_eq!(iter.next().unwrap(), (("a", Some(1.0)), IsInterpolated::No));
+        assert_eq!(iter.next().unwrap(), (("b", Some(2.0)), IsInterpolated::No));
+        assert_eq!(
+            iter.next().unwrap(),
+            (("c", Some(2.0)), IsInterpolated::Yes)
+        );
+        assert_eq!(
+            iter.next().unwrap(),
+            (("d", Some(2.0)), IsInterpolated::Yes)
+        );
         assert!(iter.next().is_none());
     }
 }
diff --git a/site/src/request_handlers.rs b/site/src/request_handlers.rs
@@ -9,7 +9,7 @@ mod status_page;
 pub use bootstrap::handle_bootstrap;
 pub use dashboard::handle_dashboard;
 pub use github::handle_github;
-pub use graph::handle_graph;
+pub use graph::handle_graphs;
 pub use next_commit::handle_next_commit;
 pub use self_profile::{
     handle_self_profile, handle_self_profile_processed_download, handle_self_profile_raw,
diff --git a/site/src/request_handlers/graph.rs b/site/src/request_handlers/graph.rs
@@ -5,11 +5,11 @@ use std::sync::Arc;
 use crate::api::graph::GraphKind;
 use crate::api::{graph, ServerResult};
 use crate::db::{self, ArtifactId, Benchmark, Profile, Scenario};
-use crate::interpolate::Interpolated;
+use crate::interpolate::IsInterpolated;
 use crate::load::SiteCtxt;
 use crate::selector::{Query, Selector, SeriesResponse, Tag};
 
-pub async fn handle_graph(
+pub async fn handle_graphs(
     body: graph::Request,
     ctxt: &SiteCtxt,
 ) -> ServerResult<Arc<graph::Response>> {
@@ -43,9 +43,9 @@ async fn graph_response(
     body: graph::Request,
     ctxt: &SiteCtxt,
 ) -> ServerResult<Arc<graph::Response>> {
-    let range = ctxt.data_range(body.start.clone()..=body.end.clone());
-    let commits: Arc<Vec<_>> = Arc::new(range.into_iter().map(|c| c.clone().into()).collect());
-    let metric_selector = Selector::One(body.stat.clone());
+    let range = ctxt.data_range(body.start..=body.end);
+    let commits: Arc<Vec<_>> = Arc::new(range.into_iter().map(|c| c.into()).collect());
+    let metric_selector = Selector::One(body.stat);
     let mut benchmarks = HashMap::new();
 
     let series_iterator = ctxt
@@ -65,7 +65,7 @@ async fn graph_response(
         let benchmark = series_response.path.get::<Benchmark>()?.to_string();
         let profile = *series_response.path.get::<Profile>()?;
         let scenario = series_response.path.get::<Scenario>()?.to_string();
-        let graph_series = graph_series(body.kind, series_response.series);
+        let graph_series = graph_series(series_response.series, body.kind);
 
         benchmarks
             .entry(benchmark)
@@ -75,14 +75,37 @@ async fn graph_response(
             .insert(scenario, graph_series);
     }
 
+    let summary_benchmark = create_summary(ctxt, metric_selector, &commits, body.kind).await?;
+
+    benchmarks.insert("Summary".to_string(), summary_benchmark);
+
+    Ok(Arc::new(graph::Response {
+        commits: Arc::try_unwrap(commits)
+            .unwrap()
+            .into_iter()
+            .map(|c| match c {
+                ArtifactId::Commit(c) => (c.date.0.timestamp(), c.sha),
+                ArtifactId::Tag(_) => unreachable!(),
+            })
+            .collect(),
+        benchmarks,
+    }))
+}
+
+/// Creates a summary "benchmark" that averages the results of all other
+/// test cases per profile type
+async fn create_summary(
+    ctxt: &SiteCtxt,
+    metric_selector: Selector<String>,
+    commits: &Arc<Vec<ArtifactId>>,
+    graph_kind: GraphKind,
+) -> ServerResult<HashMap<Profile, HashMap<String, graph::Series>>> {
     let mut baselines = HashMap::new();
     let mut summary_benchmark = HashMap::new();
-
     let summary_query_cases = iproduct!(
         ctxt.summary_scenarios(),
         vec![Profile::Check, Profile::Debug, Profile::Opt]
     );
-
     for (scenario, profile) in summary_query_cases {
         let query = Query::new()
             .set::<String>(Tag::Benchmark, Selector::All)
@@ -121,43 +144,30 @@ async fn graph_response(
         )
         .map(|((c, d), i)| ((c, Some(d.expect("interpolated") / baseline)), i));
 
-        let graph_series = graph_series(body.kind, avg_vs_baseline);
+        let graph_series = graph_series(avg_vs_baseline, graph_kind);
 
         summary_benchmark
             .entry(profile)
             .or_insert_with(HashMap::new)
             .insert(scenario.to_string(), graph_series);
     }
-
-    benchmarks.insert("Summary".to_string(), summary_benchmark);
-
-    Ok(Arc::new(graph::Response {
-        commits: Arc::try_unwrap(commits)
-            .unwrap()
-            .into_iter()
-            .map(|c| match c {
-                ArtifactId::Commit(c) => (c.date.0.timestamp(), c.sha),
-                ArtifactId::Tag(_) => unreachable!(),
-            })
-            .collect(),
-        benchmarks,
-    }))
+    Ok(summary_benchmark)
 }
 
 fn graph_series(
+    points: impl Iterator<Item = ((ArtifactId, Option<f64>), IsInterpolated)>,
     kind: GraphKind,
-    points: impl Iterator<Item = ((ArtifactId, Option<f64>), Interpolated)>,
 ) -> graph::Series {
     let mut graph_series = graph::Series {
         points: Vec::new(),
-        is_interpolated: Default::default(),
+        interpolated_indices: Default::default(),
     };
 
     let mut first = None;
     let mut prev = None;
 
-    for (idx, ((_aid, point), interpolated)) in points.enumerate() {
-        let point = point.expect("interpolated");
+    for (idx, ((_aid, point), is_interpolated)) in points.enumerate() {
+        let point = point.expect("interpolated point still produced an empty value");
         first = Some(first.unwrap_or(point));
         let first = first.unwrap();
         let percent_first = (point - first) / first * 100.0;
@@ -166,15 +176,15 @@ fn graph_series(
         prev = Some(point);
 
         let value = match kind {
-            GraphKind::Raw => point as f32,
-            GraphKind::PercentRelative => percent_prev as f32,
-            GraphKind::PercentFromFirst => percent_first as f32,
-        };
+            GraphKind::Raw => point,
+            GraphKind::PercentRelative => percent_prev,
+            GraphKind::PercentFromFirst => percent_first,
+        } as f32;
 
         graph_series.points.push(value);
 
-        if interpolated.is_interpolated() {
-            graph_series.is_interpolated.insert(idx as u16);
+        if is_interpolated.as_bool() {
+            graph_series.interpolated_indices.insert(idx as u16);
         }
     }
 
diff --git a/site/src/server.rs b/site/src/server.rs
@@ -403,16 +403,12 @@ async fn serve_req(server: Server, req: Request) -> Result<Response, ServerError
         "/perf/self-profile-raw" => Ok(to_response(
             request_handlers::handle_self_profile_raw(check!(parse_body(&body)), &ctxt).await,
         )),
-        "/perf/graph" => Ok(
-            match request_handlers::handle_graph(check!(parse_body(&body)), &ctxt).await {
+        "/perf/graphs" => Ok(
+            match request_handlers::handle_graphs(check!(parse_body(&body)), &ctxt).await {
                 Ok(result) => {
-                    let mut response = http::Response::builder()
+                    let response = http::Response::builder()
                         .header_typed(ContentType::json())
                         .header_typed(CacheControl::new().with_no_cache().with_no_store());
-                    response.headers_mut().unwrap().insert(
-                        hyper::header::ACCESS_CONTROL_ALLOW_ORIGIN,
-                        hyper::header::HeaderValue::from_static("*"),
-                    );
                     let body = serde_json::to_vec(&result).unwrap();
                     response.body(hyper::Body::from(body)).unwrap()
                 }
diff --git a/site/static/index.html b/site/static/index.html

Original file line number	Diff line number	Diff line change
`@@ -70,7 +70,7 @@ pub mod graph {`
`70`	`70`	`// y-values`
`71`	`71`	`pub points: Vec<f32>,`
`72`	`72`	`// The index of interpolated coordinates`
`73`		`- pub is_interpolated: HashSet<u16>,`
	`73`	`+ pub interpolated_indices: HashSet<u16>,`
`74`	`74`	`}`
`75`	`75`
`76`	`76`	`#[derive(Debug, PartialEq, Clone, Serialize)]`