diff --git a/Cargo.lock b/Cargo.lock index 641c828fec..73bbb638e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1380,6 +1380,7 @@ dependencies = [ "ordered-float", "percent-encoding", "pretty_assertions", + "ryu-js", "schemars", "serde", "serde_json", @@ -1649,6 +1650,7 @@ dependencies = [ "diesel_migrations", "dropshot", "http 1.4.1", + "libsqlite3-sys", "mail-send", "oso", "pretty_assertions", @@ -2763,6 +2765,7 @@ dependencies = [ "downcast-rs", "libsqlite3-sys", "r2d2", + "serde_json", "sqlite-wasm-rs", "time", ] @@ -7338,6 +7341,12 @@ version = "1.0.23" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9774ba4a74de5f7b1c1451ed6cd5285a32eddb5cccb8cc655a4e50009e06477f" +[[package]] +name = "ryu-js" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "04d056b875a9d2e6cb9a61d127afee9ac5999b9f87bcb32079d1318e505be714" + [[package]] name = "safe_arch" version = "0.7.4" diff --git a/Cargo.toml b/Cargo.toml index 1034867494..6e1c0af861 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -101,7 +101,7 @@ criterion = "0.8" css-inline = { version = "0.20", default-features = false } dashmap = "6.1" derive_more = { version = "2.1", features = ["display"] } -diesel = { version = "2.3", default-features = false, features = ["r2d2", "with-deprecated"] } +diesel = { version = "2.3", default-features = false, features = ["r2d2", "serde_json", "with-deprecated"] } diesel_migrations = "2.3" dotenvy = "0.15" email_address = "0.2" @@ -155,13 +155,18 @@ rmcp = { version = "2.2", default-features = false } rustls = { version = "0.23", default-features = false, features = ["aws_lc_rs"] } rusqlite = "0.38" rust_decimal = "1.40" +# ECMAScript `Number::toString` formatting, which is what RFC 8785 (JCS) requires +ryu-js = "1.0" schemars = { version = "0.8", features = ["uuid1"] } sentry = { version = "0.48", default-features = false, features = [ "reqwest", "rustls-no-provider", ] } serde = { version = "1.0", features = ["derive"] } -serde_json = "1.0" +# float_roundtrip: correctly rounded float parsing. Without it parsing can be a +# unit in the last place off, which makes an RFC 8785 canonical form unstable +# across a parse and re-serialize round trip. +serde_json = { version = "1.0", features = ["float_roundtrip"] } serde_urlencoded = "0.7" serde_yaml = "0.9" sha2 = "0.11" diff --git a/lib/api_projects/tests/benchmarks.rs b/lib/api_projects/tests/benchmarks.rs index 37962c0ce0..48a27a80a5 100644 --- a/lib/api_projects/tests/benchmarks.rs +++ b/lib/api_projects/tests/benchmarks.rs @@ -6,8 +6,12 @@ )] //! Integration tests for project benchmark endpoints. -use bencher_api_tests::TestServer; -use bencher_json::JsonBenchmarks; +use bencher_api_tests::{TestServer, helpers::create_empty_parameter}; +use bencher_json::{JsonBenchmark, JsonBenchmarks, JsonParameters}; +use bencher_schema::schema; +use diesel::{ + ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _, connection::SimpleConnection as _, +}; use http::StatusCode; // GET /v0/projects/{project}/benchmarks - list benchmarks (empty) @@ -129,3 +133,133 @@ async fn benchmarks_delete_not_found() { assert_eq!(resp.status(), StatusCode::NOT_FOUND); } + +// Every benchmark is born with exactly one empty parameter set, so no benchmark +// row may exist without one. +#[expect(clippy::expect_used, reason = "test assertion")] +fn assert_benchmark_birth_invariant(server: &TestServer) { + let mut conn = server.db_conn(); + let benchmark_ids: Vec = schema::benchmark::table + .select(schema::benchmark::id) + .load(&mut conn) + .expect("Failed to load benchmarks"); + assert!( + !benchmark_ids.is_empty(), + "expected at least one benchmark to check" + ); + + for benchmark_id in benchmark_ids { + let parameters: Vec = schema::parameter::table + .filter(schema::parameter::benchmark_id.eq(benchmark_id)) + .select(schema::parameter::parameters) + .load(&mut conn) + .expect("Failed to load parameters"); + assert_eq!( + parameters, + vec![JsonParameters::default()], + "benchmark {benchmark_id} must have exactly one empty parameter set" + ); + } +} + +// POST /v0/projects/{project}/benchmarks - create with the empty parameter set +#[tokio::test] +async fn benchmarks_create_empty_parameter_set() { + let server = TestServer::new().await; + let user = server + .signup("Test User", "benchmarkparameter@example.com") + .await; + let org = server.create_org(&user, "Benchmark Parameter Org").await; + let project = server + .create_project(&user, &org, "Benchmark Parameter Project") + .await; + + let project_slug: &str = project.slug.as_ref(); + let resp = server + .client + .post(server.api_url(&format!("/v0/projects/{}/benchmarks", project_slug))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&user.token), + ) + .json(&serde_json::json!({ "name": "bench one" })) + .send() + .await + .expect("Request failed"); + + assert_eq!(resp.status(), StatusCode::CREATED); + let benchmark: JsonBenchmark = resp.json().await.expect("Failed to parse response"); + + let mut conn = server.db_conn(); + let benchmark_id: i32 = schema::benchmark::table + .filter(schema::benchmark::uuid.eq(benchmark.uuid)) + .select(schema::benchmark::id) + .first(&mut conn) + .expect("Failed to get benchmark ID"); + let parameters: Vec = schema::parameter::table + .filter(schema::parameter::benchmark_id.eq(benchmark_id)) + .select(schema::parameter::parameters) + .load(&mut conn) + .expect("Failed to load parameters"); + assert_eq!(parameters, vec![JsonParameters::default()]); + + assert_benchmark_birth_invariant(&server); +} + +// POST /v0/projects/{project}/benchmarks - the benchmark insert rolls back with +// the empty parameter set insert +#[tokio::test] +async fn benchmarks_create_rolls_back_with_parameter_set() { + let server = TestServer::new().await; + let user = server + .signup("Test User", "benchmarkrollback@example.com") + .await; + let org = server.create_org(&user, "Benchmark Rollback Org").await; + let project = server + .create_project(&user, &org, "Benchmark Rollback Project") + .await; + + // Poison the empty parameter set that the next benchmark will be born with. + // SQLite hands an `INTEGER PRIMARY KEY` the next rowid after the largest in + // use, so the row below collides on `UNIQUE(benchmark_id, parameters)` with + // the set created inside the benchmark's own transaction. Foreign keys are + // off on this connection, so it may point at a benchmark that does not exist yet. + let mut conn = server.db_conn(); + let largest_benchmark_id: Option = schema::benchmark::table + .select(diesel::dsl::max(schema::benchmark::id)) + .first(&mut conn) + .expect("Failed to get the largest benchmark ID"); + let next_benchmark_id = largest_benchmark_id.unwrap_or_default() + 1; + conn.batch_execute("PRAGMA foreign_keys = OFF") + .expect("Failed to disable foreign keys"); + create_empty_parameter(&mut conn, next_benchmark_id); + conn.batch_execute("PRAGMA foreign_keys = ON") + .expect("Failed to enable foreign keys"); + + let project_slug: &str = project.slug.as_ref(); + let resp = server + .client + .post(server.api_url(&format!("/v0/projects/{}/benchmarks", project_slug))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&user.token), + ) + .json(&serde_json::json!({ "name": "bench one" })) + .send() + .await + .expect("Request failed"); + assert!( + !resp.status().is_success(), + "creating a benchmark whose parameter set collides must fail" + ); + + let benchmarks: i64 = schema::benchmark::table + .filter(schema::benchmark::name.eq("bench one")) + .count() + .get_result(&mut conn) + .expect("Failed to count benchmarks"); + assert_eq!( + benchmarks, 0, + "the benchmark insert must roll back with its parameter set insert" + ); +} diff --git a/lib/api_projects/tests/metrics.rs b/lib/api_projects/tests/metrics.rs index 852387b73c..d9021a3140 100644 --- a/lib/api_projects/tests/metrics.rs +++ b/lib/api_projects/tests/metrics.rs @@ -11,7 +11,7 @@ use bencher_api_tests::{ TestServer, - helpers::{base_timestamp, create_test_report, get_project_id}, + helpers::{base_timestamp, create_empty_parameter, create_test_report, get_project_id}, }; use bencher_json::{BenchmarkUuid, JsonOneMetric, MeasureUuid, MetricUuid, ReportBenchmarkUuid}; use bencher_schema::{ @@ -46,6 +46,7 @@ fn create_test_metric(server: &TestServer, project_id: i32, report_id: i32) -> M .select(schema::benchmark::id) .first(&mut conn) .expect("Failed to get benchmark ID"); + let parameter_id = create_empty_parameter(&mut conn, benchmark_id); // Measure let measure_uuid = MeasureUuid::new(); @@ -75,6 +76,7 @@ fn create_test_metric(server: &TestServer, project_id: i32, report_id: i32) -> M schema::report_benchmark::report_id.eq(report_id), schema::report_benchmark::iteration.eq(0), schema::report_benchmark::benchmark_id.eq(benchmark_id), + schema::report_benchmark::parameter_id.eq(parameter_id), )) .execute(&mut conn) .expect("Failed to insert report_benchmark"); diff --git a/lib/api_projects/tests/perf.rs b/lib/api_projects/tests/perf.rs index 54b77a9ff4..eec78243c8 100644 --- a/lib/api_projects/tests/perf.rs +++ b/lib/api_projects/tests/perf.rs @@ -11,7 +11,7 @@ use bencher_api_tests::{ TestServer, - helpers::{base_timestamp, get_project_id}, + helpers::{base_timestamp, create_empty_parameter, get_project_id}, }; use bencher_json::{ AlertUuid, BenchmarkUuid, BoundaryUuid, BranchUuid, HeadUuid, JobStatus, JobUuid, JsonPerf, @@ -44,6 +44,7 @@ struct PerfTestData { head_id: i32, testbed_id: i32, benchmark_id: i32, + parameter_id: i32, measure_id: i32, report_id: i32, report_benchmark_id: i32, @@ -232,6 +233,7 @@ fn create_perf_data_with_options( .select(schema::benchmark::id) .first(&mut conn) .expect("get benchmark id"); + let parameter_id = create_empty_parameter(&mut conn, benchmark_id); // Measure let measure_uuid = MeasureUuid::new(); @@ -261,6 +263,7 @@ fn create_perf_data_with_options( schema::report_benchmark::report_id.eq(report_id), schema::report_benchmark::iteration.eq(opts.iteration), schema::report_benchmark::benchmark_id.eq(benchmark_id), + schema::report_benchmark::parameter_id.eq(parameter_id), )) .execute(&mut conn) .expect("insert report_benchmark"); @@ -312,6 +315,7 @@ fn create_perf_data_with_options( head_id, testbed_id, benchmark_id, + parameter_id, measure_id, report_id, report_benchmark_id, @@ -744,6 +748,7 @@ async fn perf_get_multiple_metrics_same_permutation() { schema::report_benchmark::report_id.eq(report2_id), schema::report_benchmark::iteration.eq(0), schema::report_benchmark::benchmark_id.eq(data.benchmark_id), + schema::report_benchmark::parameter_id.eq(data.parameter_id), )) .execute(&mut conn) .expect("insert rb2"); @@ -1134,6 +1139,7 @@ async fn perf_multi_benchmark_query() { .select(schema::benchmark::id) .first(&mut conn) .expect("get benchmark2 id"); + let parameter2_id = create_empty_parameter(&mut conn, benchmark2_id); let report_benchmark2_uuid = ReportBenchmarkUuid::new(); diesel::insert_into(schema::report_benchmark::table) @@ -1142,6 +1148,7 @@ async fn perf_multi_benchmark_query() { schema::report_benchmark::report_id.eq(data.report_id), schema::report_benchmark::iteration.eq(0), schema::report_benchmark::benchmark_id.eq(benchmark2_id), + schema::report_benchmark::parameter_id.eq(parameter2_id), )) .execute(&mut conn) .expect("insert report_benchmark2"); @@ -1921,6 +1928,7 @@ async fn perf_ordered_by_version_number() { schema::report_benchmark::report_id.eq(report_v1_id), schema::report_benchmark::iteration.eq(0), schema::report_benchmark::benchmark_id.eq(data_v2.benchmark_id), + schema::report_benchmark::parameter_id.eq(data_v2.parameter_id), )) .execute(&mut conn) .expect("insert rb v1"); @@ -2052,6 +2060,7 @@ async fn perf_ordered_by_start_time_within_version() { schema::report_benchmark::report_id.eq(r_id), schema::report_benchmark::iteration.eq(0), schema::report_benchmark::benchmark_id.eq(data.benchmark_id), + schema::report_benchmark::parameter_id.eq(data.parameter_id), )) .execute(&mut conn) .expect("insert rb"); @@ -2540,6 +2549,7 @@ async fn perf_multiple_iterations() { schema::report_benchmark::report_id.eq(data.report_id), schema::report_benchmark::iteration.eq(1), schema::report_benchmark::benchmark_id.eq(data.benchmark_id), + schema::report_benchmark::parameter_id.eq(data.parameter_id), )) .execute(&mut conn) .expect("insert rb iter1"); @@ -2861,6 +2871,7 @@ async fn perf_spec_filters_results() { schema::report_benchmark::report_id.eq(report2_id), schema::report_benchmark::iteration.eq(0), schema::report_benchmark::benchmark_id.eq(data1.benchmark_id), + schema::report_benchmark::parameter_id.eq(data1.parameter_id), )) .execute(&mut conn) .expect("insert rb2"); diff --git a/lib/api_projects/tests/projects.rs b/lib/api_projects/tests/projects.rs index 3055ee956e..7fa154d799 100644 --- a/lib/api_projects/tests/projects.rs +++ b/lib/api_projects/tests/projects.rs @@ -710,10 +710,10 @@ struct PlotDimensions { #[expect(clippy::expect_used, reason = "test helper seeding plot dimensions")] fn seed_plot_dimensions(server: &TestServer, project_id: i32) -> PlotDimensions { - use bencher_api_tests::helpers::base_timestamp; + use bencher_api_tests::helpers::{base_timestamp, create_empty_parameter}; use bencher_json::{BenchmarkUuid, BranchUuid, MeasureUuid, TestbedUuid}; use bencher_schema::schema; - use diesel::{ExpressionMethods as _, RunQueryDsl as _}; + use diesel::{ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; let now = base_timestamp(); let branch1 = BranchUuid::new(); @@ -758,6 +758,12 @@ fn seed_plot_dimensions(server: &TestServer, project_id: i32) -> PlotDimensions )) .execute(&mut conn) .expect("Failed to insert benchmark"); + let benchmark_id: i32 = schema::benchmark::table + .filter(schema::benchmark::uuid.eq(&benchmark)) + .select(schema::benchmark::id) + .first(&mut conn) + .expect("Failed to get benchmark ID"); + create_empty_parameter(&mut conn, benchmark_id); diesel::insert_into(schema::measure::table) .values(( schema::measure::uuid.eq(&measure), diff --git a/lib/api_projects/tests/reports.rs b/lib/api_projects/tests/reports.rs index 64dc37d223..56fca4760b 100644 --- a/lib/api_projects/tests/reports.rs +++ b/lib/api_projects/tests/reports.rs @@ -10,11 +10,14 @@ use bencher_api_tests::{ TestServer, - helpers::{base_timestamp, create_test_report, get_project_id}, + helpers::{ + base_timestamp, create_empty_parameter, create_test_report, get_empty_parameter, + get_project_id, + }, }; use bencher_json::{ - BenchmarkUuid, BoundaryUuid, JsonReport, JsonReports, MeasureUuid, MetricUuid, ModelUuid, - ReportBenchmarkUuid, ThresholdUuid, + BenchmarkUuid, BoundaryUuid, JsonParameters, JsonReport, JsonReports, MeasureUuid, MetricUuid, + ModelUuid, ReportBenchmarkUuid, ThresholdUuid, }; use bencher_schema::{ context::DbConnection, @@ -52,6 +55,7 @@ fn seed_result_infra( .select(schema::benchmark::id) .first(&mut *conn) .expect("Failed to get benchmark ID"); + create_empty_parameter(conn, benchmark_id); let measure_uuid = MeasureUuid::new(); diesel::insert_into(schema::measure::table) @@ -137,6 +141,7 @@ fn seed_report_results(server: &TestServer, project_id: i32, report_id: i32, cou let (benchmark_id, measure_id, threshold_id, model_id) = seed_result_infra(&mut conn, project_id, branch_id, testbed_id); + let parameter_id = get_empty_parameter(&mut conn, benchmark_id); let report_benchmarks = (0..count) .map(|iteration| { @@ -145,6 +150,7 @@ fn seed_report_results(server: &TestServer, project_id: i32, report_id: i32, cou schema::report_benchmark::report_id.eq(report_id), schema::report_benchmark::iteration.eq(iteration), schema::report_benchmark::benchmark_id.eq(benchmark_id), + schema::report_benchmark::parameter_id.eq(parameter_id), ) }) .collect::>(); @@ -589,3 +595,65 @@ async fn reports_delete_not_found() { assert_eq!(resp.status(), StatusCode::NOT_FOUND); } + +// POST /v0/projects/{project}/reports - ingest creates each benchmark with its +// empty parameter set and rides every result on it +#[tokio::test] +async fn reports_ingest_empty_parameter_sets() { + let server = TestServer::new().await; + let user = server + .signup("Test User", "reportparameter@example.com") + .await; + let org = server.create_org(&user, "Report Parameter Org").await; + let project = server + .create_project(&user, &org, "Report Parameter Project") + .await; + + let project_slug: &str = project.slug.as_ref(); + let report = post_report(&server, &user.token, project_slug).await; + assert_eq!( + report.results.as_ref().map(Vec::len), + Some(2), + "two iterations were ingested" + ); + + let mut conn = server.db_conn(); + let benchmark_ids: Vec = schema::benchmark::table + .select(schema::benchmark::id) + .load(&mut conn) + .expect("Failed to load benchmarks"); + assert_eq!(benchmark_ids.len(), 2, "two benchmarks were ingested"); + + for benchmark_id in benchmark_ids { + let parameters: Vec = schema::parameter::table + .filter(schema::parameter::benchmark_id.eq(benchmark_id)) + .select(schema::parameter::parameters) + .load(&mut conn) + .expect("Failed to load parameters"); + assert_eq!( + parameters, + vec![JsonParameters::default()], + "benchmark {benchmark_id} must have exactly one empty parameter set" + ); + } + + let report_benchmarks: Vec<(i32, i32)> = schema::report_benchmark::table + .select(( + schema::report_benchmark::benchmark_id, + schema::report_benchmark::parameter_id, + )) + .load(&mut conn) + .expect("Failed to load report benchmarks"); + assert_eq!( + report_benchmarks.len(), + 4, + "two iterations of two benchmarks" + ); + for (benchmark_id, parameter_id) in report_benchmarks { + assert_eq!( + parameter_id, + get_empty_parameter(&mut conn, benchmark_id), + "every report benchmark rides its own benchmark's empty parameter set" + ); + } +} diff --git a/lib/bencher_adapter/src/adapters/java/jmh.rs b/lib/bencher_adapter/src/adapters/java/jmh.rs index 36e386fc86..e449ddc063 100644 --- a/lib/bencher_adapter/src/adapters/java/jmh.rs +++ b/lib/bencher_adapter/src/adapters/java/jmh.rs @@ -245,7 +245,7 @@ pub(crate) mod test_java_jmh { .unwrap(); validate_throughput( metrics, - 113_640_916.672_629_92, + 113_640_916.672_629_9, Some(105_176_321.973_520_52), Some(122_105_511.371_739_3), ); diff --git a/lib/bencher_adapter/src/adapters/shell/hyperfine.rs b/lib/bencher_adapter/src/adapters/shell/hyperfine.rs index bfc0255177..4f4fa22de7 100644 --- a/lib/bencher_adapter/src/adapters/shell/hyperfine.rs +++ b/lib/bencher_adapter/src/adapters/shell/hyperfine.rs @@ -162,7 +162,7 @@ pub(crate) mod test_shell_hyperfine { let metrics = results.get("sleep 0.1").unwrap(); validate_latency( metrics, - 106_525_351.72, + 106_525_351.720_000_01, Some(102_474_685.72), Some(115_336_892.72), ); @@ -170,9 +170,9 @@ pub(crate) mod test_shell_hyperfine { let metrics = results.get("sleep 0.2").unwrap(); validate_latency( metrics, - 208_661_518.72, - Some(201_824_142.72), - Some(214_128_684.72), + 208_661_518.720_000_03, + Some(201_824_142.720_000_03), + Some(214_128_684.720_000_03), ); } @@ -184,9 +184,9 @@ pub(crate) mod test_shell_hyperfine { let metrics = results.get("sleep 0.01").unwrap(); validate_latency( metrics, - 13_317_239.025_420_565, - Some(12_317_546.734_914_13), - Some(14_316_931.315_926_999), + 13_317_239.025_420_563, + Some(12_317_546.734_914_128), + Some(14_316_931.315_926_997), ); } @@ -200,7 +200,7 @@ pub(crate) mod test_shell_hyperfine { metrics, 13_251_329.96, Some(10_165_892.459_999_999), - Some(21_347_058.459_999_997), + Some(21_347_058.46), ); } } diff --git a/lib/bencher_api_tests/src/helpers.rs b/lib/bencher_api_tests/src/helpers.rs index c5a38bb220..63a1155a28 100644 --- a/lib/bencher_api_tests/src/helpers.rs +++ b/lib/bencher_api_tests/src/helpers.rs @@ -3,10 +3,10 @@ //! These helpers are used by both `api_projects` and `api_runners` integration tests. use bencher_json::{ - BranchUuid, DateTime, HeadUuid, JobStatus, JobUuid, Jwt, ReportUuid, ResourceName, TestbedUuid, - TokenUuid, VersionUuid, + BranchUuid, DateTime, HeadUuid, JobStatus, JobUuid, JsonParameters, Jwt, ParameterUuid, + ReportUuid, ResourceName, TestbedUuid, TokenUuid, VersionUuid, }; -use bencher_schema::{model::user::UserId, schema}; +use bencher_schema::{context::DbConnection, model::user::UserId, schema}; use diesel::{ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; use crate::{TestServer, seed::TestUser}; @@ -29,6 +29,44 @@ pub fn get_project_id(server: &TestServer, project_slug: &str) -> i32 { .expect("Failed to get project ID") } +/// Create the empty parameter set that every benchmark is born with. +/// +/// Benchmarks inserted directly into the database bypass `QueryBenchmark::create`, +/// so they need the birth invariant applied by hand. +#[expect(clippy::expect_used, reason = "test helper inserting a parameter set")] +pub fn create_empty_parameter(conn: &mut DbConnection, benchmark_id: i32) -> i32 { + let now = base_timestamp(); + + let parameter_uuid = ParameterUuid::new(); + diesel::insert_into(schema::parameter::table) + .values(( + schema::parameter::uuid.eq(¶meter_uuid), + schema::parameter::benchmark_id.eq(benchmark_id), + schema::parameter::parameters.eq(JsonParameters::default()), + schema::parameter::created.eq(&now), + schema::parameter::modified.eq(&now), + )) + .execute(&mut *conn) + .expect("Failed to insert parameter"); + + schema::parameter::table + .filter(schema::parameter::uuid.eq(¶meter_uuid)) + .select(schema::parameter::id) + .first(&mut *conn) + .expect("Failed to get parameter ID") +} + +/// Get a benchmark's empty parameter set. +#[expect(clippy::expect_used, reason = "test helper querying a parameter set")] +pub fn get_empty_parameter(conn: &mut DbConnection, benchmark_id: i32) -> i32 { + schema::parameter::table + .filter(schema::parameter::benchmark_id.eq(benchmark_id)) + .filter(schema::parameter::parameters.eq(JsonParameters::default())) + .select(schema::parameter::id) + .first(&mut *conn) + .expect("Failed to get empty parameter set") +} + /// Create minimal test infrastructure (testbed, version, branch, head, report). /// Returns the report ID. Uses a deterministic timestamp. #[expect( diff --git a/lib/bencher_json/Cargo.toml b/lib/bencher_json/Cargo.toml index ee0632ea92..d4de65285f 100644 --- a/lib/bencher_json/Cargo.toml +++ b/lib/bencher_json/Cargo.toml @@ -12,7 +12,9 @@ client = ["bencher_context/client", "bencher_valid/client"] table = ["dep:tabled"] server = ["bencher_context/server", "bencher_valid/server"] schema = ["dep:schemars", "bencher_context/schema", "ordered-float/schemars"] -db = ["dep:diesel", "dep:serde_yaml", "bencher_valid/db"] +# `diesel/sqlite`: the parameter set encoder is SQLite's JSONB format, +# so its `ToSql` and `FromSql` are written against the SQLite backend. +db = ["dep:diesel", "diesel/sqlite", "dep:serde_yaml", "bencher_valid/db"] plus = ["dep:camino", "bencher_valid/plus"] test-clock = ["bencher_valid/test-clock"] @@ -24,6 +26,7 @@ derive_more.workspace = true diesel = { workspace = true, optional = true } ordered-float = { workspace = true, features = ["serde"] } percent-encoding.workspace = true +ryu-js.workspace = true schemars = { workspace = true, optional = true, features = ["chrono", "url"] } serde.workspace = true serde_json.workspace = true diff --git a/lib/bencher_json/src/lib.rs b/lib/bencher_json/src/lib.rs index c6f95cf5fd..d7fddd47cf 100644 --- a/lib/bencher_json/src/lib.rs +++ b/lib/bencher_json/src/lib.rs @@ -86,6 +86,7 @@ pub use project::{ JsonMetric, JsonMetricsMap, JsonNewMetric, JsonOneMetric, JsonResultsMap, MetricUuid, }, model::{JsonModel, ModelUuid}, + parameter::{JsonParameters, ParameterUuid}, perf::{JsonPerf, JsonPerfQuery, ReportBenchmarkUuid}, plot::{JsonNewPlot, JsonPlot, JsonPlots, PlotUuid}, report::{ diff --git a/lib/bencher_json/src/project/mod.rs b/lib/bencher_json/src/project/mod.rs index 248ff3c8a6..86b5499558 100644 --- a/lib/bencher_json/src/project/mod.rs +++ b/lib/bencher_json/src/project/mod.rs @@ -22,6 +22,7 @@ pub mod key; pub mod measure; pub mod metric; pub mod model; +pub mod parameter; pub mod perf; pub mod plot; pub mod report; diff --git a/lib/bencher_json/src/project/parameter/jsonb.rs b/lib/bencher_json/src/project/parameter/jsonb.rs new file mode 100644 index 0000000000..2efcb5b824 --- /dev/null +++ b/lib/bencher_json/src/project/parameter/jsonb.rs @@ -0,0 +1,402 @@ +//! `SQLite`'s [JSONB][jsonb] binary encoding. +//! +//! Two writers reach the `parameter.parameters` column: this encoder, and +//! `SQLite`'s own `jsonb()` in the migration that mints the empty parameter set. +//! `UNIQUE(benchmark_id, parameters)` compares bytes, so the two have to agree +//! byte for byte, and `SQLite` is the definition of correct. Where the format +//! admits more than one encoding of the same value, this matches what `jsonb()` +//! produces over the RFC 8785 (JCS) canonical text of the same parameter set. +//! +//! An element is a header followed by a payload. The header's low nibble is the +//! element type. Its high nibble is the payload size when the size is at most +//! 11, and otherwise selects a big endian size that follows the header, in the +//! most compact of the one, two, and four byte forms. +//! +//! [jsonb]: https://sqlite.org/jsonb.html + +use super::{write_json_string, write_json_string_body}; + +/// `null`. +const NULL: u8 = 0x00; +/// `true`. +const TRUE: u8 = 0x01; +/// `false`. +const FALSE: u8 = 0x02; +/// An integer, as JSON spells it. +const INT: u8 = 0x03; +/// An integer in a JSON5 notation, which canonical JSON never produces. +const INT5: u8 = 0x04; +/// A float, as JSON spells it. +const FLOAT: u8 = 0x05; +/// A float in a JSON5 notation, which canonical JSON never produces. +const FLOAT5: u8 = 0x06; +/// A string whose source text holds no escapes. +const TEXT: u8 = 0x07; +/// A string whose source text holds JSON escapes, carried through verbatim. +const TEXTJ: u8 = 0x08; +/// A string holding JSON5 escapes, which canonical JSON never produces. +const TEXT5: u8 = 0x09; +/// SQL text that has to be escaped to become JSON. +const TEXTRAW: u8 = 0x0a; +/// An array. +const ARRAY: u8 = 0x0b; +/// An object. +const OBJECT: u8 = 0x0c; + +/// The largest payload size a header can carry on its own. +const INLINE_SIZE: u32 = 11; +/// The high nibble marking a one byte payload size. +const ONE_BYTE_SIZE: u8 = 0xc0; +/// The high nibble marking a two byte payload size. +const TWO_BYTE_SIZE: u8 = 0xd0; +/// The high nibble marking a four byte payload size. +const FOUR_BYTE_SIZE: u8 = 0xe0; + +/// How deep a blob may nest before it is treated as malformed. +/// +/// A parameter set is an object of scalars, so anything deeper than that is +/// already corruption. The limit is what keeps the decoder off the stack. +const MAX_DEPTH: u8 = 32; + +/// A JSONB object under construction. +/// +/// Members are appended in the order they are given, which for a parameter set +/// is the RFC 8785 key order. The encoder never sorts. +#[derive(Debug, Default)] +pub struct Object(Vec); + +impl Object { + /// Append a `null` member. + pub fn insert_null(&mut self, key: &str) -> Result<(), JsonbError> { + self.insert(key, NULL, &[]) + } + + /// Append a boolean member. + pub fn insert_bool(&mut self, key: &str, value: bool) -> Result<(), JsonbError> { + self.insert(key, if value { TRUE } else { FALSE }, &[]) + } + + /// Append a number member. + /// + /// The payload is the canonical number text, written through unchanged. + /// `SQLite`'s parser reads a number as a float when its text holds a fraction + /// or an exponent and as an integer otherwise, whatever its magnitude, so an + /// integer above `i64` is still an integer here. + pub fn insert_number(&mut self, key: &str, canonical: &str) -> Result<(), JsonbError> { + let element = if canonical.contains(['.', 'e', 'E']) { + FLOAT + } else { + INT + }; + self.insert(key, element, canonical.as_bytes()) + } + + /// Append a string member. + pub fn insert_string(&mut self, key: &str, value: &str) -> Result<(), JsonbError> { + let (element, payload) = string_element(value); + self.insert(key, element, payload.as_bytes()) + } + + fn insert(&mut self, key: &str, element: u8, payload: &[u8]) -> Result<(), JsonbError> { + let (key_element, key_payload) = string_element(key); + push_element(&mut self.0, key_element, key_payload.as_bytes())?; + push_element(&mut self.0, element, payload) + } + + /// The JSONB encoding of the object. + pub fn into_blob(self) -> Result, JsonbError> { + let mut blob = Vec::with_capacity(self.0.len().saturating_add(5)); + push_element(&mut blob, OBJECT, &self.0)?; + Ok(blob) + } +} + +/// A string's element type and the payload that goes with it. +/// +/// The payload is the string as it appears between the quotes of the canonical +/// text, so a string that needs no escape is carried literally and a string that +/// does is carried escaped. `SQLite` reads a string as `TEXT` until it meets a +/// backslash, at which point the element becomes `TEXTJ`. +fn string_element(string: &str) -> (u8, String) { + let mut payload = String::new(); + write_json_string_body(string, &mut payload); + let element = if payload.contains('\\') { TEXTJ } else { TEXT }; + (element, payload) +} + +/// Append one element: a header, then its payload. +fn push_element(blob: &mut Vec, element: u8, payload: &[u8]) -> Result<(), JsonbError> { + // SQLite holds payload sizes in a `u32`, so anything larger has no encoding + // to agree with rather than one this could guess at. + let size = u32::try_from(payload.len()).map_err(JsonbError::PayloadSize)?; + if size <= INLINE_SIZE { + blob.push(element | size_nibble(size)); + } else if u8::try_from(size).is_ok() { + blob.push(element | ONE_BYTE_SIZE); + blob.push(size_byte(size, 0)); + } else if u16::try_from(size).is_ok() { + blob.push(element | TWO_BYTE_SIZE); + blob.push(size_byte(size, 8)); + blob.push(size_byte(size, 0)); + } else { + blob.push(element | FOUR_BYTE_SIZE); + blob.push(size_byte(size, 24)); + blob.push(size_byte(size, 16)); + blob.push(size_byte(size, 8)); + blob.push(size_byte(size, 0)); + } + blob.extend_from_slice(payload); + Ok(()) +} + +/// A payload size small enough to ride in the header's high nibble. +/// +/// The caller only reaches this with a size at most the inline maximum, so the +/// mask takes the whole of it. Masking first is what puts the shift in the `u8` +/// domain, where a nibble cannot outgrow the byte it moves into. +const fn size_nibble(size: u32) -> u8 { + ((size & 0x0f) as u8) << 4 +} + +/// One byte of a big endian payload size. +const fn size_byte(size: u32, shift: u32) -> u8 { + ((size >> shift) & 0xff) as u8 +} + +/// Render a JSONB blob as JSON text. +/// +/// Both writers land in the same column, so this reads back what this encoder +/// wrote and what `SQLite`'s `jsonb()` wrote. +pub fn to_json(blob: &[u8]) -> Result { + let mut json = String::new(); + let index = write_value(blob, 0, 0, &mut json)?; + if index == blob.len() { + Ok(json) + } else { + Err(JsonbError::TrailingBytes) + } +} + +/// Write the element at `index` as JSON text and return the index just past it. +fn write_value( + blob: &[u8], + index: usize, + depth: u8, + json: &mut String, +) -> Result { + if depth > MAX_DEPTH { + return Err(JsonbError::TooDeep); + } + let header = *blob.get(index).ok_or(JsonbError::Truncated)?; + let (size, header_size) = payload_size(blob, index, header >> 4)?; + let start = index + .checked_add(header_size) + .ok_or(JsonbError::Truncated)?; + let end = start.checked_add(size).ok_or(JsonbError::Truncated)?; + let payload = blob.get(start..end).ok_or(JsonbError::Truncated)?; + + match header & 0x0f { + NULL => json.push_str("null"), + TRUE => json.push_str("true"), + FALSE => json.push_str("false"), + // The payload is the number as JSON spells it, so it needs no parsing + // step here: the magnitude is the caller's problem, not the codec's. + INT | FLOAT => json.push_str(text(payload)?), + TEXT | TEXTRAW => write_json_string(text(payload)?, json), + TEXTJ => { + json.push('"'); + json.push_str(text(payload)?); + json.push('"'); + }, + ARRAY => write_array(blob, start, end, depth, json)?, + OBJECT => write_object(blob, start, end, depth, json)?, + element @ (INT5 | FLOAT5 | TEXT5) => return Err(JsonbError::Json5(element)), + element => return Err(JsonbError::Element(element)), + } + Ok(end) +} + +fn write_array( + blob: &[u8], + start: usize, + end: usize, + depth: u8, + json: &mut String, +) -> Result<(), JsonbError> { + json.push('['); + let mut index = start; + while index < end { + if index > start { + json.push(','); + } + index = write_value(blob, index, depth.saturating_add(1), json)?; + } + if index == end { + json.push(']'); + Ok(()) + } else { + Err(JsonbError::Truncated) + } +} + +fn write_object( + blob: &[u8], + start: usize, + end: usize, + depth: u8, + json: &mut String, +) -> Result<(), JsonbError> { + json.push('{'); + let mut index = start; + while index < end { + if index > start { + json.push(','); + } + index = write_value(blob, index, depth.saturating_add(1), json)?; + if index >= end { + return Err(JsonbError::Truncated); + } + json.push(':'); + index = write_value(blob, index, depth.saturating_add(1), json)?; + } + if index == end { + json.push('}'); + Ok(()) + } else { + Err(JsonbError::Truncated) + } +} + +/// The payload size an element header describes, and the size of that header. +fn payload_size(blob: &[u8], index: usize, marker: u8) -> Result<(usize, usize), JsonbError> { + let size_bytes = match marker { + 0..=11 => return Ok((usize::from(marker), 1)), + 12 => 1, + 13 => 2, + 14 => 4, + // 15, the eight byte form, which SQLite's own encoder never writes. + _ => 8, + }; + let start = index.checked_add(1).ok_or(JsonbError::Truncated)?; + let mut size = 0usize; + for offset in 0..size_bytes { + let byte = *blob + .get(start.checked_add(offset).ok_or(JsonbError::Truncated)?) + .ok_or(JsonbError::Truncated)?; + size = size + .checked_mul(0x100) + .and_then(|size| size.checked_add(usize::from(byte))) + .ok_or(JsonbError::PayloadTooLarge)?; + } + let header_size = size_bytes.checked_add(1).ok_or(JsonbError::Truncated)?; + Ok((size, header_size)) +} + +fn text(payload: &[u8]) -> Result<&str, JsonbError> { + std::str::from_utf8(payload).map_err(JsonbError::Utf8) +} + +#[derive(Debug, thiserror::Error)] +pub enum JsonbError { + #[error("Failed to encode a JSONB payload: {0}")] + PayloadSize(std::num::TryFromIntError), + #[error("JSONB payload is larger than this platform can address")] + PayloadTooLarge, + #[error("JSONB blob ends inside an element")] + Truncated, + #[error("JSONB blob has bytes after its value")] + TrailingBytes, + #[error("JSONB blob nests deeper than a JSON value can")] + TooDeep, + #[error("JSONB element ({0:#04x}) is JSON5, which is not JSON")] + Json5(u8), + #[error("JSONB element ({0:#04x}) is not a JSON value")] + Element(u8), + #[error("JSONB payload is not UTF-8: {0}")] + Utf8(std::str::Utf8Error), +} + +#[cfg(test)] +mod tests { + use super::{JsonbError, Object, to_json}; + + fn object() -> Object { + Object::default() + } + + #[test] + fn empty_object_is_one_byte() { + let blob = object().into_blob().expect("Failed to encode"); + assert_eq!(blob, vec![0x0c], "the empty object is a single header byte"); + assert_eq!(to_json(&blob).expect("Failed to decode"), "{}"); + } + + #[test] + fn scalars_round_trip() { + let mut object = object(); + object.insert_bool("debug", true).expect("Failed to encode"); + object.insert_null("gap").expect("Failed to encode"); + object + .insert_number("threads", "4") + .expect("Failed to encode"); + object + .insert_number("tolerance", "1e-7") + .expect("Failed to encode"); + object + .insert_string("path", "C:\\bench\\x") + .expect("Failed to encode"); + object + .insert_string("os", "linux") + .expect("Failed to encode"); + let blob = object.into_blob().expect("Failed to encode"); + + assert_eq!( + to_json(&blob).expect("Failed to decode"), + r#"{"debug":true,"gap":null,"threads":4,"tolerance":1e-7,"path":"C:\\bench\\x","os":"linux"}"#, + "every scalar survives a round trip in the order it was inserted" + ); + } + + #[test] + fn payload_larger_than_the_inline_size() { + // A payload of 12 bytes or more moves the size out of the header nibble. + let mut object = object(); + object + .insert_string("k", &"x".repeat(300)) + .expect("Failed to encode"); + let blob = object.into_blob().expect("Failed to encode"); + assert_eq!( + to_json(&blob).expect("Failed to decode"), + format!(r#"{{"k":"{}"}}"#, "x".repeat(300)), + "a payload past the inline size still round trips" + ); + } + + #[test] + fn malformed_blobs_are_rejected() { + assert!( + matches!(to_json(&[]), Err(JsonbError::Truncated)), + "an empty blob has no value in it" + ); + // An object header claiming three payload bytes that are not there. + assert!( + matches!(to_json(&[0x3c]), Err(JsonbError::Truncated)), + "a header without its payload is truncated" + ); + // A valid `{}` followed by a byte that belongs to no element. + assert!( + matches!(to_json(&[0x0c, 0x0c]), Err(JsonbError::TrailingBytes)), + "bytes after the value are not part of it" + ); + // JSON5 elements, which canonical JSON never produces. + assert!( + matches!(to_json(&[0x14, 0x30]), Err(JsonbError::Json5(0x04))), + "JSON5 is not JSON" + ); + // An object whose payload ends between a key and its value. + assert!( + matches!(to_json(&[0x2c, 0x17, 0x78]), Err(JsonbError::Truncated)), + "an object member without its value is truncated" + ); + } +} diff --git a/lib/bencher_json/src/project/parameter/mod.rs b/lib/bencher_json/src/project/parameter/mod.rs new file mode 100644 index 0000000000..596cc9f904 --- /dev/null +++ b/lib/bencher_json/src/project/parameter/mod.rs @@ -0,0 +1,608 @@ +use std::{cmp::Ordering, collections::BTreeMap, fmt, str::FromStr}; + +use ordered_float::OrderedFloat; +#[cfg(feature = "schema")] +use schemars::JsonSchema; +use serde::{Deserialize, Deserializer, Serialize, Serializer, de, ser::SerializeMap as _}; + +#[cfg(feature = "db")] +pub mod jsonb; + +crate::typed_uuid::typed_uuid!(ParameterUuid); + +/// A benchmark parameter set: the permutation of inputs that a benchmark ran with. +/// +/// The canonical form is [RFC 8785][jcs] (JSON Canonicalization Scheme): +/// object keys sorted by UTF-16 code unit, ECMAScript number formatting, +/// and no insignificant whitespace. +/// Canonicalization happens here, before the write, +/// so the database's `UNIQUE(benchmark_id, parameters)` constraint +/// is the enforcement point for canonical equality. +/// +/// [jcs]: https://www.rfc-editor.org/rfc/rfc8785 +#[derive(Debug, Clone, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[cfg_attr(feature = "db", derive(diesel::FromSqlRow, diesel::AsExpression))] +#[cfg_attr(feature = "db", diesel(sql_type = diesel::sql_types::Jsonb))] +pub struct JsonParameters(BTreeMap); + +impl JsonParameters { + /// The RFC 8785 (JCS) canonical serialization of this parameter set. + pub fn canonical(&self) -> String { + let mut canonical = String::from("{"); + for (index, (key, value)) in self.0.iter().enumerate() { + if index > 0 { + canonical.push(','); + } + write_json_string(key.as_ref(), &mut canonical); + canonical.push(':'); + match value { + ParameterValue::Bool(boolean) => { + canonical.push_str(if *boolean { "true" } else { "false" }); + }, + ParameterValue::Number(number) => { + canonical.push_str(ryu_js::Buffer::new().format(number.into_inner())); + }, + ParameterValue::String(string) => write_json_string(string, &mut canonical), + } + } + canonical.push('}'); + canonical + } + + /// Whether this is the empty parameter set. + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } + + /// The `SQLite` JSONB encoding of the canonical form. + /// + /// Byte identical to what `SQLite`'s own `jsonb()` produces over + /// [`Self::canonical`], which is what lets a set written here collide with a + /// set minted in SQL on `UNIQUE(benchmark_id, parameters)`. + #[cfg(feature = "db")] + pub fn to_jsonb(&self) -> Result, jsonb::JsonbError> { + let mut object = jsonb::Object::default(); + for (key, value) in &self.0 { + match value { + ParameterValue::Bool(boolean) => object.insert_bool(key.as_ref(), *boolean)?, + ParameterValue::Number(number) => object.insert_number( + key.as_ref(), + ryu_js::Buffer::new().format(number.into_inner()), + )?, + ParameterValue::String(string) => object.insert_string(key.as_ref(), string)?, + } + } + object.into_blob() + } +} + +impl fmt::Display for JsonParameters { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.canonical()) + } +} + +impl FromStr for JsonParameters { + type Err = ParametersError; + + fn from_str(parameters: &str) -> Result { + serde_json::from_str(parameters).map_err(ParametersError::Json) + } +} + +impl Serialize for JsonParameters { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + let mut map = serializer.serialize_map(Some(self.0.len()))?; + for (key, value) in &self.0 { + map.serialize_entry(key, value)?; + } + map.end() + } +} + +impl<'de> Deserialize<'de> for JsonParameters { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + BTreeMap::deserialize(deserializer).map(Self) + } +} + +#[derive(Debug, thiserror::Error)] +pub enum ParametersError { + #[error("Failed to parse benchmark parameters: {0}")] + Json(serde_json::Error), +} + +/// A parameter set key, ordered by UTF-16 code unit as RFC 8785 requires. +/// +/// That order is not the code point order: a supplementary plane character +/// (U+10000 and above) leads with a surrogate in U+D800..U+DBFF, so it sorts +/// before every character in U+E000..U+FFFF. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +struct ParameterKey(String); + +impl AsRef for ParameterKey { + fn as_ref(&self) -> &str { + &self.0 + } +} + +impl Ord for ParameterKey { + fn cmp(&self, other: &Self) -> Ordering { + self.0.encode_utf16().cmp(other.0.encode_utf16()) + } +} + +impl PartialOrd for ParameterKey { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Serialize for ParameterKey { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(&self.0) + } +} + +impl<'de> Deserialize<'de> for ParameterKey { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + String::deserialize(deserializer).map(Self) + } +} + +/// A parameter set value: a JSON scalar. +/// +/// Null, arrays, and objects are rejected. Numbers are ECMAScript doubles, +/// so `16`, `16.0`, and `1.6e1` are one value with one canonical spelling. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +enum ParameterValue { + Bool(bool), + Number(OrderedFloat), + String(String), +} + +impl Serialize for ParameterValue { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + match self { + Self::Bool(boolean) => serializer.serialize_bool(*boolean), + // Round trip through the canonical spelling so an integral value + // goes out as `16` and not `16.0`. + Self::Number(number) => { + let canonical = ryu_js::Buffer::new().format(number.into_inner()).to_owned(); + serde_json::from_str::(&canonical) + .map_err(serde::ser::Error::custom)? + .serialize(serializer) + }, + Self::String(string) => serializer.serialize_str(string), + } + } +} + +impl<'de> Deserialize<'de> for ParameterValue { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + deserializer.deserialize_any(ParameterValueVisitor) + } +} + +struct ParameterValueVisitor; + +impl ParameterValueVisitor { + fn number(number: &serde_json::Number) -> Result + where + E: de::Error, + { + // Deferred to `serde_json` so the integer to double conversion that + // RFC 8785 specifies happens in one place. + number + .as_f64() + .filter(|number| number.is_finite()) + .map(|number| ParameterValue::Number(OrderedFloat(number))) + .ok_or_else(|| E::custom(format!("Parameter value ({number}) is not a finite number"))) + } +} + +impl de::Visitor<'_> for ParameterValueVisitor { + type Value = ParameterValue; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("a JSON scalar parameter value (string, number, or boolean)") + } + + fn visit_bool(self, v: bool) -> Result + where + E: de::Error, + { + Ok(ParameterValue::Bool(v)) + } + + fn visit_i64(self, v: i64) -> Result + where + E: de::Error, + { + Self::number(&serde_json::Number::from(v)) + } + + fn visit_u64(self, v: u64) -> Result + where + E: de::Error, + { + Self::number(&serde_json::Number::from(v)) + } + + fn visit_f64(self, v: f64) -> Result + where + E: de::Error, + { + serde_json::Number::from_f64(v).map_or_else( + || { + Err(E::custom(format!( + "Parameter value ({v}) is not a finite number" + ))) + }, + |number| Self::number(&number), + ) + } + + fn visit_str(self, v: &str) -> Result + where + E: de::Error, + { + Ok(ParameterValue::String(v.to_owned())) + } + + fn visit_string(self, v: String) -> Result + where + E: de::Error, + { + Ok(ParameterValue::String(v)) + } +} + +/// Append the RFC 8785 escaping of a JSON string, which is the escaping +/// ECMAScript `JSON.stringify` performs: the seven short escapes, `\u00xx` in +/// lowercase hex for the remaining control characters, and everything else literal. +fn write_json_string(string: &str, canonical: &mut String) { + canonical.push('"'); + write_json_string_body(string, canonical); + canonical.push('"'); +} + +/// Append the body of an RFC 8785 escaped JSON string, without its quotes. +fn write_json_string_body(string: &str, canonical: &mut String) { + for character in string.chars() { + match character { + '"' => canonical.push_str("\\\""), + '\\' => canonical.push_str("\\\\"), + '\u{8}' => canonical.push_str("\\b"), + '\u{9}' => canonical.push_str("\\t"), + '\u{a}' => canonical.push_str("\\n"), + '\u{c}' => canonical.push_str("\\f"), + '\u{d}' => canonical.push_str("\\r"), + control if control < '\u{20}' => { + let control = u32::from(control); + canonical.push_str("\\u00"); + canonical.push(hex_digit(control >> 4)); + canonical.push(hex_digit(control & 0xf)); + }, + character => canonical.push(character), + } + } +} + +fn hex_digit(nibble: u32) -> char { + char::from_digit(nibble, 16).unwrap_or('0') +} + +#[cfg(test)] +mod tests { + use ordered_float::OrderedFloat; + + use super::{JsonParameters, ParameterKey, ParameterValue}; + + fn canonical(parameters: &str) -> String { + parameters + .parse::() + .expect("Failed to parse parameters") + .canonical() + } + + /// A one key parameter set holding an exact `f64`, built without a parsing + /// step so a bit pattern reaches the canonicalizer unchanged. + fn number(value: f64) -> JsonParameters { + JsonParameters( + [( + ParameterKey("n".to_owned()), + ParameterValue::Number(OrderedFloat(value)), + )] + .into_iter() + .collect(), + ) + } + + #[test] + fn canonical_empty_set() { + assert_eq!(canonical("{}"), "{}"); + assert_eq!(JsonParameters::default().canonical(), "{}"); + } + + #[test] + fn canonical_no_insignificant_whitespace() { + assert_eq!( + canonical("{\n \"size_mb\": 16,\n \"op\": \"read\"\n}"), + r#"{"op":"read","size_mb":16}"# + ); + } + + #[test] + fn canonical_key_order_is_utf16_code_units() { + // RFC 8785 section 3.2.3. The emoji is U+1F600, which sorts *before* + // U+FB33 by UTF-16 code unit (its lead surrogate is U+D83D) and *after* + // it by code point. A UTF-8 or code point sort silently gets this wrong. + let parameters = canonical( + r#"{ + "\u20ac": "Euro Sign", + "\r": "Carriage Return", + "\ufb33": "Hebrew Letter Dalet With Dagesh", + "1": "One", + "\ud83d\ude00": "Emoji: Grinning Face", + "\u0080": "Control", + "\u00f6": "Latin Small Letter O With Diaeresis" + }"#, + ); + assert_eq!( + parameters, + concat!( + r#"{"\r":"Carriage Return","1":"One","#, + "\"\u{80}\":\"Control\",", + "\"\u{f6}\":\"Latin Small Letter O With Diaeresis\",", + "\"\u{20ac}\":\"Euro Sign\",", + "\"\u{1f600}\":\"Emoji: Grinning Face\",", + "\"\u{fb33}\":\"Hebrew Letter Dalet With Dagesh\"}" + ) + ); + } + + #[test] + fn canonical_string_escapes() { + // The seven short escapes are preserved. + assert_eq!( + canonical(r#"{"a": "\b\t\n\f\r\"\\"}"#), + r#"{"a":"\b\t\n\f\r\"\\"}"# + ); + // Control characters without a short escape use lowercase hex. + assert_eq!( + canonical(r#"{"a": "\u001F\u0000"}"#), + r#"{"a":"\u001f\u0000"}"# + ); + // Everything else is literal, including the solidus and DEL. + assert_eq!(canonical(r#"{"a": "/\u007f"}"#), "{\"a\":\"/\u{7f}\"}"); + } + + #[test] + fn canonical_booleans() { + assert_eq!( + canonical(r#"{"b": false, "a": true}"#), + r#"{"a":true,"b":false}"# + ); + } + + // RFC 8785 appendix B: ECMAScript `Number::toString` formatting. + #[test] + fn canonical_number_formatting() { + for (parameters, expected) in [ + (r#"{"n": 0}"#, r#"{"n":0}"#), + (r#"{"n": -0.0}"#, r#"{"n":0}"#), + (r#"{"n": 5e-324}"#, r#"{"n":5e-324}"#), + (r#"{"n": -5e-324}"#, r#"{"n":-5e-324}"#), + ( + r#"{"n": 1.7976931348623157e308}"#, + r#"{"n":1.7976931348623157e+308}"#, + ), + ( + r#"{"n": -1.7976931348623157e308}"#, + r#"{"n":-1.7976931348623157e+308}"#, + ), + (r#"{"n": 9007199254740992}"#, r#"{"n":9007199254740992}"#), + (r#"{"n": -9007199254740992}"#, r#"{"n":-9007199254740992}"#), + ( + r#"{"n": 295147905179352825856}"#, + r#"{"n":295147905179352830000}"#, + ), + (r#"{"n": 1e21}"#, r#"{"n":1e+21}"#), + (r#"{"n": 1e23}"#, r#"{"n":1e+23}"#), + (r#"{"n": 0.000001}"#, r#"{"n":0.000001}"#), + ( + r#"{"n": 9.999999999999997e-7}"#, + r#"{"n":9.999999999999997e-7}"#, + ), + (r#"{"n": 333333333.3333333}"#, r#"{"n":333333333.3333333}"#), + (r#"{"n": 1}"#, r#"{"n":1}"#), + (r#"{"n": -1.5}"#, r#"{"n":-1.5}"#), + (r#"{"n": 100}"#, r#"{"n":100}"#), + ] { + assert_eq!(canonical(parameters), expected, "for {parameters}"); + } + } + + // RFC 8785 appendix B: every IEEE 754 value and its ECMAScript + // `Number::toString` serialization, keyed by bit pattern so the table is read + // exactly as the RFC states it, with no parsing step in between. + #[test] + fn canonical_number_conformance() { + for (bits, expected) in [ + (0x0000_0000_0000_0000u64, "0"), + (0x8000_0000_0000_0000u64, "0"), + (0x0000_0000_0000_0001u64, "5e-324"), + (0x8000_0000_0000_0001u64, "-5e-324"), + (0x7fef_ffff_ffff_ffffu64, "1.7976931348623157e+308"), + (0xffef_ffff_ffff_ffffu64, "-1.7976931348623157e+308"), + (0x4340_0000_0000_0000u64, "9007199254740992"), + (0xc340_0000_0000_0000u64, "-9007199254740992"), + (0x4430_0000_0000_0000u64, "295147905179352830000"), + (0x44b5_2d02_c7e1_4af5u64, "9.999999999999997e+22"), + (0x44b5_2d02_c7e1_4af6u64, "1e+23"), + (0x44b5_2d02_c7e1_4af7u64, "1.0000000000000001e+23"), + (0x444b_1ae4_d6e2_ef4eu64, "999999999999999700000"), + (0x444b_1ae4_d6e2_ef4fu64, "999999999999999900000"), + (0x444b_1ae4_d6e2_ef50u64, "1e+21"), + (0x3eb0_c6f7_a0b5_ed8cu64, "9.999999999999997e-7"), + (0x3eb0_c6f7_a0b5_ed8du64, "0.000001"), + (0x41b3_de43_5555_5553u64, "333333333.3333332"), + (0x41b3_de43_5555_5554u64, "333333333.33333325"), + (0x41b3_de43_5555_5555u64, "333333333.3333333"), + (0x41b3_de43_5555_5556u64, "333333333.3333334"), + (0x41b3_de43_5555_5557u64, "333333333.33333343"), + (0xbecb_f647_612f_3696u64, "-0.0000033333333333333333"), + // Round to even, where the shortest round trip digits are a tie. + (0x4314_3ff3_c1cb_0959u64, "1424953923781206.2"), + ] { + assert_eq!( + number(f64::from_bits(bits)).canonical(), + format!(r#"{{"n":{expected}}}"#), + "for {bits:016x}" + ); + } + } + + // The canonical form has to survive a write and a read: a parameter set read + // back out of the database is parsed, and re-canonicalizing it must land on + // the same bytes or `UNIQUE(benchmark_id, parameters)` stops holding. + #[test] + fn canonical_survives_a_round_trip() { + // Deterministic xorshift64, so the sample never varies between runs. + let mut bits: u64 = 0x2545_f491_4f6c_dd1d; + for _ in 0..10_000u32 { + bits ^= bits << 13; + bits ^= bits >> 7; + bits ^= bits << 17; + let value = f64::from_bits(bits); + if !value.is_finite() { + continue; + } + + let once = number(value).canonical(); + let twice = once + .parse::() + .expect("Failed to parse canonical parameters") + .canonical(); + assert_eq!(once, twice, "for {bits:016x}"); + } + } + + #[test] + fn canonical_number_spellings_collapse() { + let sixteen = canonical(r#"{"n": 16}"#); + assert_eq!(sixteen, r#"{"n":16}"#); + assert_eq!(canonical(r#"{"n": 16.0}"#), sixteen); + assert_eq!(canonical(r#"{"n": 1.6e1}"#), sixteen); + } + + #[test] + fn canonical_key_order_collapses() { + assert_eq!( + canonical(r#"{"b": 1, "a": 2}"#), + canonical(r#"{"a": 2, "b": 1}"#) + ); + } + + // A duplicate key is last wins, which is what parsing into a map does and what + // every JSON parser a harness is likely to use already does. That is the decided + // behavior, not an accident: it is the least bad of the options, the easiest to + // explain, and it just works. A duplicate is never an error. + // + // Two keys that differ only in case are not duplicates. They are two keys, and + // RFC 8785 orders `A` before `a` by UTF-16 code unit. + #[test] + fn duplicate_keys_are_last_wins() { + assert_eq!(canonical(r#"{"a": 1, "a": 2}"#), r#"{"a":2}"#); + assert_eq!(canonical(r#"{"a": 1, "a": "two"}"#), r#"{"a":"two"}"#); + assert_eq!(canonical(r#"{"A": 1, "a": 2}"#), r#"{"A":1,"a":2}"#); + } + + #[test] + fn canonical_round_trips_through_parsing() { + let parameters = canonical(r#"{"size_mb": 16, "op": "read", "fsync": true}"#); + assert_eq!(canonical(¶meters), parameters); + } + + #[test] + fn rejects_non_scalar_values() { + for parameters in [ + r#"{"a": null}"#, + r#"{"a": []}"#, + r#"{"a": [1, 2]}"#, + r#"{"a": {}}"#, + r#"{"a": {"b": 1}}"#, + ] { + assert!( + parameters.parse::().is_err(), + "expected {parameters} to be rejected" + ); + } + } + + #[test] + fn rejects_non_object_payloads() { + for parameters in ["null", "[]", "1", r#""a""#, "true"] { + assert!( + parameters.parse::().is_err(), + "expected {parameters} to be rejected" + ); + } + } + + #[test] + fn accepts_scalar_values() { + let parameters = r#"{"s": "a", "n": 1, "b": true}"# + .parse::() + .expect("Failed to parse parameters"); + assert!(!parameters.is_empty()); + } +} + +/// The JSONB encoding is `SQLite`'s, so these impls are too. +/// +/// Every other backend spells `Jsonb` differently, and a generic impl would be +/// claiming an encoding it does not have. +#[cfg(feature = "db")] +mod db { + use super::{JsonParameters, jsonb}; + + impl diesel::serialize::ToSql for JsonParameters { + fn to_sql<'b>( + &'b self, + out: &mut diesel::serialize::Output<'b, '_, diesel::sqlite::Sqlite>, + ) -> diesel::serialize::Result { + out.set_value(self.to_jsonb()?); + Ok(diesel::serialize::IsNull::No) + } + } + + impl diesel::deserialize::FromSql + for JsonParameters + { + fn from_sql( + mut bytes: diesel::sqlite::SqliteValue<'_, '_, '_>, + ) -> diesel::deserialize::Result { + Ok(jsonb::to_json(bytes.read_blob())?.parse()?) + } + } +} diff --git a/lib/bencher_schema/Cargo.toml b/lib/bencher_schema/Cargo.toml index db1453452e..1f3de940b9 100644 --- a/lib/bencher_schema/Cargo.toml +++ b/lib/bencher_schema/Cargo.toml @@ -61,6 +61,10 @@ diesel = { workspace = true, features = ["chrono", "sqlite", "32-column-tables"] diesel_migrations.workspace = true dropshot.workspace = true http = { workspace = true, optional = true } +# The migrations mint JSONB with SQLite's own `jsonb()`, which needs SQLite 3.45 +# or later, so the crate that owns them pins the bundled amalgamation rather than +# linking whatever the host happens to ship. +libsqlite3-sys.workspace = true mail-send.workspace = true regex.workspace = true rusqlite = { workspace = true, features = ["backup"] } diff --git a/lib/bencher_schema/migrations/2026-08-15-120000_benchmark_parameter/down.sql b/lib/bencher_schema/migrations/2026-08-15-120000_benchmark_parameter/down.sql new file mode 100644 index 0000000000..5270b2603c --- /dev/null +++ b/lib/bencher_schema/migrations/2026-08-15-120000_benchmark_parameter/down.sql @@ -0,0 +1,32 @@ +PRAGMA foreign_keys = off; +-- report_benchmark +CREATE TABLE down_report_benchmark ( + id INTEGER PRIMARY KEY NOT NULL, + uuid TEXT NOT NULL UNIQUE, + report_id INTEGER NOT NULL, + iteration INTEGER NOT NULL, + benchmark_id INTEGER NOT NULL, + FOREIGN KEY (report_id) REFERENCES report (id) ON DELETE CASCADE, + FOREIGN KEY (benchmark_id) REFERENCES benchmark (id), + UNIQUE(report_id, iteration, benchmark_id) +); +INSERT INTO down_report_benchmark( + id, + uuid, + report_id, + iteration, + benchmark_id + ) +SELECT id, + uuid, + report_id, + iteration, + benchmark_id +FROM report_benchmark; +DROP TABLE report_benchmark; +ALTER TABLE down_report_benchmark + RENAME TO report_benchmark; +CREATE INDEX index_report_benchmark_benchmark_report ON report_benchmark(benchmark_id, report_id); +-- parameter +DROP TABLE parameter; +PRAGMA foreign_keys = on; diff --git a/lib/bencher_schema/migrations/2026-08-15-120000_benchmark_parameter/up.sql b/lib/bencher_schema/migrations/2026-08-15-120000_benchmark_parameter/up.sql new file mode 100644 index 0000000000..613ac53a85 --- /dev/null +++ b/lib/bencher_schema/migrations/2026-08-15-120000_benchmark_parameter/up.sql @@ -0,0 +1,83 @@ +PRAGMA foreign_keys = off; +-- parameter +-- `parameters` holds the SQLite JSONB encoding of the RFC 8785 (JCS) canonical +-- form of the parameter set, so `UNIQUE(benchmark_id, parameters)` is the +-- enforcement point for canonical equality. It is declared `BLOB` to match the +-- SQLite representation of the `Jsonb` SQL type, and SQLite's JSON functions +-- read it without a parse step. +CREATE TABLE parameter ( + id INTEGER PRIMARY KEY NOT NULL, + uuid TEXT NOT NULL UNIQUE, + benchmark_id INTEGER NOT NULL, + parameters BLOB NOT NULL, + created BIGINT NOT NULL, + modified BIGINT NOT NULL, + archived BIGINT, + FOREIGN KEY (benchmark_id) REFERENCES benchmark (id) ON DELETE CASCADE, + UNIQUE(benchmark_id, parameters) +); +-- Every benchmark is born with its empty parameter set, so every benchmark that +-- predates this migration is backfilled with one. +-- Pure SQL has no UUIDv7 function, so the UUID is a v4 minted from `randomblob`: +-- 16 random bytes with the version nibble set to 4 and the variant nibble drawn +-- from `89ab`. `random() & 3` is used rather than `abs(random()) % 4` because +-- `abs(-9223372036854775808)` is an integer overflow error in SQLite. +-- The empty set is minted with `jsonb()` so that SQLite itself defines the bytes +-- the encoder in `bencher_json` has to reproduce. +INSERT INTO parameter(uuid, benchmark_id, parameters, created, modified) +SELECT lower( + hex(randomblob(4)) || '-' || hex(randomblob(2)) || '-4' || substr(hex(randomblob(2)), 2) || '-' || substr('89ab', (random() & 3) + 1, 1) || substr(hex(randomblob(2)), 2) || '-' || hex(randomblob(6)) + ), + id, + jsonb('{}'), + created, + modified +FROM benchmark; +-- report_benchmark +-- `parameter_id` is NOT NULL: SQLite unique indexes treat NULLs as distinct, so a +-- nullable dimension would silently void `UNIQUE(report_id, iteration, benchmark_id, parameter_id)`. +CREATE TABLE up_report_benchmark ( + id INTEGER PRIMARY KEY NOT NULL, + uuid TEXT NOT NULL UNIQUE, + report_id INTEGER NOT NULL, + iteration INTEGER NOT NULL, + benchmark_id INTEGER NOT NULL, + parameter_id INTEGER NOT NULL, + FOREIGN KEY (report_id) REFERENCES report (id) ON DELETE CASCADE, + FOREIGN KEY (benchmark_id) REFERENCES benchmark (id), + FOREIGN KEY (parameter_id) REFERENCES parameter (id), + UNIQUE(report_id, iteration, benchmark_id, parameter_id) +); +-- The join is a `LEFT JOIN` so that a `report_benchmark` row whose benchmark has +-- no empty parameter set trips the `NOT NULL` on `parameter_id` and fails the +-- migration. Every benchmark is backfilled above, so this cannot fire on valid +-- data; an `INNER JOIN` would drop such a row silently instead. +INSERT INTO up_report_benchmark( + id, + uuid, + report_id, + iteration, + benchmark_id, + parameter_id + ) +SELECT report_benchmark.id, + report_benchmark.uuid, + report_benchmark.report_id, + report_benchmark.iteration, + report_benchmark.benchmark_id, + parameter.id +FROM report_benchmark + LEFT JOIN parameter ON ( + parameter.benchmark_id = report_benchmark.benchmark_id + AND parameter.parameters = jsonb('{}') + ); +DROP TABLE report_benchmark; +ALTER TABLE up_report_benchmark + RENAME TO report_benchmark; +CREATE INDEX index_report_benchmark_benchmark_report ON report_benchmark(benchmark_id, report_id); +-- `benchmark` cascades to `parameter`, so deleting a benchmark (or a project) +-- deletes parameter sets, and SQLite then verifies that no `report_benchmark` +-- row still references them. Without this index that check is a full table scan +-- of `report_benchmark` per deleted parameter set. +CREATE INDEX index_report_benchmark_parameter ON report_benchmark(parameter_id); +PRAGMA foreign_keys = on; diff --git a/lib/bencher_schema/src/error.rs b/lib/bencher_schema/src/error.rs index 92cd8c31aa..4bfa777454 100644 --- a/lib/bencher_schema/src/error.rs +++ b/lib/bencher_schema/src/error.rs @@ -27,6 +27,7 @@ pub enum BencherResource { HeadVersion, Testbed, Benchmark, + Parameter, Measure, Metric, Threshold, @@ -72,6 +73,7 @@ impl fmt::Display for BencherResource { Self::HeadVersion => "Head Version", Self::Testbed => "Testbed", Self::Benchmark => "Benchmark", + Self::Parameter => "Parameter", Self::Measure => "Measure", Self::Metric => "Metric", Self::Threshold => "Threshold", diff --git a/lib/bencher_schema/src/lib.rs b/lib/bencher_schema/src/lib.rs index cf7de07802..a4af1c9d5a 100644 --- a/lib/bencher_schema/src/lib.rs +++ b/lib/bencher_schema/src/lib.rs @@ -2,6 +2,8 @@ use criterion as _; use diesel::connection::SimpleConnection as _; use diesel_migrations::{EmbeddedMigrations, MigrationHarness as _, embed_migrations}; +// Linked for the bundled SQLite amalgamation, which the migrations need for `jsonb()`. +use libsqlite3_sys as _; pub mod context; pub mod error; diff --git a/lib/bencher_schema/src/model/project/benchmark.rs b/lib/bencher_schema/src/model/project/benchmark.rs index 6efca613f1..5f406f3567 100644 --- a/lib/bencher_schema/src/model/project/benchmark.rs +++ b/lib/bencher_schema/src/model/project/benchmark.rs @@ -5,7 +5,7 @@ use bencher_json::{ use diesel::{ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; use dropshot::HttpError; -use super::{ProjectId, QueryProject}; +use super::{ProjectId, QueryProject, parameter::InsertParameter}; use crate::{ auth_conn, context::{ApiContext, DbConnection}, @@ -121,11 +121,22 @@ impl QueryBenchmark { let insert_benchmark = InsertBenchmark::from_json(auth_conn!(context), project_id, json_benchmark); + // A benchmark is born with its empty parameter set, in the benchmark's own + // transaction, so no benchmark ever exists without one. `write_transaction!` + // does not nest, so `create` must never be called from inside another one. write_transaction!(context, |conn| { diesel::insert_into(schema::benchmark::table) .values(&insert_benchmark) .execute(conn)?; - diesel::select(last_insert_rowid()).get_result(conn) + let benchmark_id: BenchmarkId = diesel::select(last_insert_rowid()).get_result(conn)?; + + let insert_parameter = + InsertParameter::empty_set(benchmark_id, insert_benchmark.created); + diesel::insert_into(schema::parameter::table) + .values(&insert_parameter) + .execute(conn)?; + + diesel::QueryResult::Ok(benchmark_id) }) .map_err(resource_conflict_err!(Benchmark, &insert_benchmark)) .map(|id| insert_benchmark.into_query(id)) diff --git a/lib/bencher_schema/src/model/project/mod.rs b/lib/bencher_schema/src/model/project/mod.rs index 0170e0df2c..d021953ebb 100644 --- a/lib/bencher_schema/src/model/project/mod.rs +++ b/lib/bencher_schema/src/model/project/mod.rs @@ -44,6 +44,7 @@ pub mod key; pub mod measure; pub mod metric; pub mod metric_boundary; +pub mod parameter; pub mod plot; pub mod project_role; pub mod report; diff --git a/lib/bencher_schema/src/model/project/parameter.rs b/lib/bencher_schema/src/model/project/parameter.rs new file mode 100644 index 0000000000..ee436062d5 --- /dev/null +++ b/lib/bencher_schema/src/model/project/parameter.rs @@ -0,0 +1,732 @@ +use bencher_json::{DateTime, JsonParameters, ParameterUuid}; +use diesel::{ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; +use dropshot::HttpError; + +use crate::{ + context::DbConnection, + error::issue_error, + macros::fn_get::{fn_from_uuid, fn_get, fn_get_id, fn_get_uuid}, + schema::{self, parameter as parameter_table}, +}; + +use super::benchmark::{BenchmarkId, QueryBenchmark}; + +crate::macros::typed_id::typed_id!(ParameterId); + +/// A parameter set: one grid point under its benchmark. +/// +/// Parameter sets have neither a name nor a slug, so they are UUID addressed only, +/// following the `report` and `alert` precedent. +#[derive( + Debug, Clone, diesel::Queryable, diesel::Identifiable, diesel::Associations, diesel::Selectable, +)] +#[diesel(table_name = parameter_table)] +#[diesel(belongs_to(QueryBenchmark, foreign_key = benchmark_id))] +pub struct QueryParameter { + pub id: ParameterId, + pub uuid: ParameterUuid, + pub benchmark_id: BenchmarkId, + pub parameters: JsonParameters, + pub created: DateTime, + pub modified: DateTime, + pub archived: Option, +} + +impl QueryParameter { + fn_get!(parameter, ParameterId); + fn_get_id!(parameter, ParameterId, ParameterUuid); + fn_get_uuid!(parameter, ParameterId, ParameterUuid); + fn_from_uuid!( + benchmark_id, + BenchmarkId, + parameter, + ParameterUuid, + Parameter + ); + + /// Get the benchmark's empty parameter set. + /// + /// Every benchmark is created atomically with its empty parameter set, + /// so a missing row is data corruption and not a missing get-or-create. + pub fn get_empty_set_id( + conn: &mut DbConnection, + benchmark_id: BenchmarkId, + ) -> Result { + schema::parameter::table + .filter(schema::parameter::benchmark_id.eq(benchmark_id)) + .filter(schema::parameter::parameters.eq(JsonParameters::default())) + .select(schema::parameter::id) + .first(conn) + .map_err(|e| { + let message = format!( + "Failed to query the empty parameter set for benchmark ({benchmark_id})" + ); + issue_error(&message, &message, e) + }) + } +} + +#[derive(Debug, diesel::Insertable)] +#[diesel(table_name = parameter_table)] +pub struct InsertParameter { + pub uuid: ParameterUuid, + pub benchmark_id: BenchmarkId, + pub parameters: JsonParameters, + pub created: DateTime, + pub modified: DateTime, + pub archived: Option, +} + +impl InsertParameter { + /// The empty parameter set that every benchmark is born with. + /// + /// The timestamp is the benchmark's own creation timestamp: + /// the parameter set is created in the same transaction as its benchmark. + pub fn empty_set(benchmark_id: BenchmarkId, timestamp: DateTime) -> Self { + Self { + uuid: ParameterUuid::new(), + benchmark_id, + parameters: JsonParameters::default(), + created: timestamp, + modified: timestamp, + archived: None, + } + } + + pub fn into_query(self, id: ParameterId) -> QueryParameter { + let Self { + uuid, + benchmark_id, + parameters, + created, + modified, + archived, + } = self; + QueryParameter { + id, + uuid, + benchmark_id, + parameters, + created, + modified, + archived, + } + } +} + +#[derive(Debug, Clone, diesel::AsChangeset)] +#[diesel(table_name = parameter_table)] +pub struct UpdateParameter { + pub parameters: Option, + pub modified: DateTime, + pub archived: Option>, +} + +#[cfg(test)] +mod tests { + use bencher_json::{DateTime, JsonParameters, ParameterUuid}; + use diesel::{ + ExpressionMethods as _, QueryDsl as _, QueryResult, RunQueryDsl as _, SqliteConnection, + connection::SimpleConnection as _, + }; + use diesel_migrations::MigrationHarness as _; + + use bencher_json::project::parameter::jsonb; + + use crate::{ + model::project::benchmark::BenchmarkId, + schema, + test_util::{ + create_base_entities, create_benchmark, create_parameter, get_empty_parameter, + setup_test_db, + }, + }; + + /// Where the blob under test comes from. + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + enum Encode { + /// Written through the `parameter.parameters` column: the production path. + Column, + /// Encoded directly. Parameter values are scalar only, so a null value + /// never reaches the column, but the encoder still has to agree with `SQLite`. + Encoder, + } + + /// Every parameter set that has to encode to the same bytes as `SQLite`'s `jsonb()`. + /// + /// Each entry is already in its RFC 8785 (JCS) canonical form. The set covers + /// the shapes a parameter value can take: strings that need JSON escapes and + /// strings that do not, exponent form floats, integers above `i64`, control and + /// supplementary plane characters, and key orders where the UTF-16 sort differs + /// from the UTF-8 one. + const CONFORMANCE: &[(&str, &str, Encode)] = &[ + ("empty", "{}", Encode::Column), + ( + "realistic", + r#"{"label":"say \"hi\"","path":"C:\\bench\\x","tolerance":1e-7}"#, + Encode::Column, + ), + ( + "scalars", + r#"{"debug":true,"os":"linux","threads":4}"#, + Encode::Column, + ), + ("null", r#"{"x":null}"#, Encode::Encoder), + ("float-simple", r#"{"a":0.1,"z":16.5}"#, Encode::Column), + ("float-tiny", r#"{"b":1e-7}"#, Encode::Column), + ("float-huge", r#"{"c":1e+21}"#, Encode::Column), + ("float-min-sub", r#"{"d":5e-324}"#, Encode::Column), + ( + "float-max", + r#"{"e":1.7976931348623157e+308}"#, + Encode::Column, + ), + ("big-int", r#"{"n":10000000000000000000}"#, Encode::Column), + ("int-2p53", r#"{"n":9007199254740992}"#, Encode::Column), + ("neg", r#"{"n":-1}"#, Encode::Column), + ("str-quote", r#"{"q":"say \"hi\""}"#, Encode::Column), + ("str-backslash", r#"{"s":"a\\b"}"#, Encode::Column), + ("str-newline", r#"{"s":"x\ny"}"#, Encode::Column), + ("str-tab", r#"{"s":"a\tb"}"#, Encode::Column), + ("str-del", "{\"s\":\"a\u{7f}b\"}", Encode::Column), + ("str-unicode", r#"{"s":"héllo"}"#, Encode::Column), + ( + "nonbmp-keys", + "{\"\u{1f600}\":1,\"\u{fb33}\":2}", + Encode::Column, + ), + ]; + + #[derive(diesel::QueryableByName)] + struct SqlText { + #[diesel(sql_type = diesel::sql_types::Text)] + value: String, + } + + #[derive(diesel::QueryableByName)] + struct SqlInteger { + #[diesel(sql_type = diesel::sql_types::Integer)] + value: i32, + } + + fn parameters(parameters: &str) -> JsonParameters { + parameters.parse().expect("Failed to parse parameters") + } + + fn hex(blob: &[u8]) -> String { + use std::fmt::Write as _; + + blob.iter().fold(String::new(), |mut hex, byte| { + write!(hex, "{byte:02X}").expect("Failed to format a byte"); + hex + }) + } + + /// The bytes `SQLite`'s own `jsonb()` produces for a canonical text. + fn sqlite_jsonb(conn: &mut SqliteConnection, canonical: &str) -> String { + diesel::sql_query("SELECT hex(jsonb(?)) AS value") + .bind::(canonical) + .get_result::(conn) + .expect("Failed to mint a JSONB blob") + .value + } + + /// A scalar SQL expression over one stored parameter set. + fn parameter_text( + conn: &mut SqliteConnection, + parameter_id: super::ParameterId, + sql: &str, + ) -> String { + diesel::sql_query(format!("SELECT {sql} AS value FROM parameter WHERE id = ?")) + .bind::(parameter_id) + .get_result::(conn) + .expect("Failed to read the parameter set") + .value + } + + fn parameter_integer( + conn: &mut SqliteConnection, + parameter_id: super::ParameterId, + sql: &str, + ) -> i32 { + diesel::sql_query(format!("SELECT {sql} AS value FROM parameter WHERE id = ?")) + .bind::(parameter_id) + .get_result::(conn) + .expect("Failed to read the parameter set") + .value + } + + /// A scalar SQL expression over a blob bound directly, for encoded bytes that + /// never reach the column. + fn blob_text(conn: &mut SqliteConnection, sql: &str, blob: Vec) -> String { + diesel::sql_query(format!("SELECT {sql} AS value")) + .bind::(blob) + .get_result::(conn) + .expect("Failed to read the encoded parameter set") + .value + } + + fn blob_integer(conn: &mut SqliteConnection, sql: &str, blob: Vec) -> i32 { + diesel::sql_query(format!("SELECT {sql} AS value")) + .bind::(blob) + .get_result::(conn) + .expect("Failed to read the encoded parameter set") + .value + } + + /// Mint a parameter set with `SQLite`'s `jsonb()`, the migration's write path. + fn mint_parameter( + conn: &mut SqliteConnection, + benchmark_id: BenchmarkId, + canonical: &str, + ) -> QueryResult { + diesel::sql_query( + "INSERT INTO parameter(uuid, benchmark_id, parameters, created, modified) + VALUES (?, ?, jsonb(?), 0, 0)", + ) + .bind::(ParameterUuid::new().to_string()) + .bind::(benchmark_id) + .bind::(canonical) + .execute(conn) + } + + fn is_unique_violation(result: &QueryResult) -> bool { + matches!( + result, + Err(diesel::result::Error::DatabaseError( + diesel::result::DatabaseErrorKind::UniqueViolation, + _ + )) + ) + } + + /// Whether a write either landed or collided on the unique constraint, + /// as opposed to failing for some other reason. + fn landed_or_collided(result: &QueryResult) -> bool { + match result { + Ok(_) => true, + Err(_) => is_unique_violation(result), + } + } + + /// The parameter set a benchmark was born with, or a freshly written one. + fn write_parameter( + conn: &mut SqliteConnection, + benchmark_id: BenchmarkId, + parameters: &JsonParameters, + ) -> super::ParameterId { + if parameters.is_empty() { + get_empty_parameter(conn, benchmark_id) + } else { + create_parameter(conn, benchmark_id, parameters) + } + } + + // The encoder has to be byte identical to SQLite's `jsonb()` over the same + // canonical text, because `UNIQUE(benchmark_id, parameters)` compares bytes and + // both writers reach that column: the migration mints the empty set with + // `jsonb('{}')` and everything after that is written through Diesel. + #[test] + fn byte_agreement_with_sqlite_jsonb() { + let mut conn = setup_test_db(); + let base = create_base_entities(&mut conn); + let benchmark_id = create_benchmark( + &mut conn, + base.project_id, + "00000000-0000-0000-0000-000000000010", + "bench1", + "bench1", + ); + + for (name, canonical, encode) in CONFORMANCE { + let minted = sqlite_jsonb(&mut conn, canonical); + match encode { + Encode::Column => { + let parameters = parameters(canonical); + assert_eq!( + parameters.canonical(), + *canonical, + "{name}: the conformance text is already canonical" + ); + + let parameter_id = write_parameter(&mut conn, benchmark_id, ¶meters); + assert_eq!( + parameter_text(&mut conn, parameter_id, "hex(parameters)"), + minted, + "{name}: the written bytes must be the bytes jsonb() mints" + ); + assert_eq!( + parameter_integer(&mut conn, parameter_id, "json_valid(parameters, 8)"), + 1, + "{name}: SQLite's JSON functions must accept the written bytes" + ); + assert_eq!( + parameter_text(&mut conn, parameter_id, "json(parameters)"), + *canonical, + "{name}: the canonical text must survive the column unchanged" + ); + }, + Encode::Encoder => { + // `{"x":null}`. Scalar only validation rejects a null value, so + // this one set is encoded directly rather than through the column. + // The column write, the unique collision and the `FromSql` round + // trip are the only assertions that need the column, so the bytes + // are bound directly and still checked against `jsonb()`, + // `json_valid()` and `json()`. + let mut object = jsonb::Object::default(); + object + .insert_null("x") + .expect("Failed to encode a null member"); + let blob = object.into_blob().expect("Failed to encode the object"); + assert_eq!( + hex(&blob), + minted, + "{name}: the encoded bytes must be the bytes jsonb() mints" + ); + assert_eq!( + blob_integer(&mut conn, "json_valid(?, 8)", blob.clone()), + 1, + "{name}: SQLite's JSON functions must accept the encoded bytes" + ); + assert_eq!( + blob_text(&mut conn, "json(?)", blob), + *canonical, + "{name}: the canonical text must survive the encoder unchanged" + ); + }, + } + } + } + + // A parameter set read back out of the column has to be the set that was + // written, whichever writer wrote it, and re-canonicalizing it has to land on + // the same text or the unique constraint stops holding. + #[test] + fn parameters_read_back_from_both_writers() { + let mut conn = setup_test_db(); + let base = create_base_entities(&mut conn); + let written = create_benchmark( + &mut conn, + base.project_id, + "00000000-0000-0000-0000-000000000010", + "bench1", + "bench1", + ); + let minted = create_benchmark( + &mut conn, + base.project_id, + "00000000-0000-0000-0000-000000000011", + "bench2", + "bench2", + ); + // SQLite mints every set under this benchmark, the empty one included, so + // the Diesel written set it was born with is cleared out of the way first. + diesel::delete(schema::parameter::table.filter(schema::parameter::benchmark_id.eq(minted))) + .execute(&mut conn) + .expect("Failed to clear the minted benchmark's parameter sets"); + + for (name, canonical, encode) in CONFORMANCE { + if *encode != Encode::Column { + continue; + } + let parameters = parameters(canonical); + + let parameter_id = write_parameter(&mut conn, written, ¶meters); + let read: JsonParameters = schema::parameter::table + .filter(schema::parameter::id.eq(parameter_id)) + .select(schema::parameter::parameters) + .first(&mut conn) + .expect("Failed to read back a written parameter set"); + assert_eq!(read, parameters, "{name}: written and read back"); + assert_eq!( + read.canonical(), + *canonical, + "{name}: written stays canonical" + ); + + mint_parameter(&mut conn, minted, canonical).expect("Failed to mint a parameter set"); + let read: JsonParameters = schema::parameter::table + .filter(schema::parameter::benchmark_id.eq(minted)) + .order(schema::parameter::id.desc()) + .select(schema::parameter::parameters) + .first(&mut conn) + .expect("Failed to read back a minted parameter set"); + assert_eq!(read, parameters, "{name}: minted and read back"); + assert_eq!( + read.canonical(), + *canonical, + "{name}: minted stays canonical" + ); + } + } + + // The unique constraint is the enforcement point, so a set written through + // Diesel and the same set minted by `jsonb()` have to collide on it. + #[test] + fn write_paths_collide_on_unique() { + let mut conn = setup_test_db(); + let base = create_base_entities(&mut conn); + + for (index, (name, canonical, encode)) in CONFORMANCE.iter().enumerate() { + if *encode != Encode::Column { + continue; + } + let benchmark_id = create_benchmark( + &mut conn, + base.project_id, + &format!("00000000-0000-0000-0000-{index:012}"), + &format!("bench{index}"), + &format!("bench{index}"), + ); + + // The empty set is already there, written through Diesel when the + // benchmark was born, so for that one set the mint is what collides. + let minted = mint_parameter(&mut conn, benchmark_id, canonical); + let written = insert_parameter(&mut conn, benchmark_id, ¶meters(canonical)); + + assert!( + is_unique_violation(&minted) || is_unique_violation(&written), + "{name}: the two writers must collide on UNIQUE(benchmark_id, parameters)" + ); + assert!( + landed_or_collided(&minted), + "{name}: the mint must either land or collide" + ); + assert!( + landed_or_collided(&written), + "{name}: the write must either land or collide" + ); + + let expected = if *canonical == "{}" { 1 } else { 2 }; + assert_eq!( + count_parameters(&mut conn, benchmark_id), + expected, + "{name}: one row per distinct parameter set" + ); + } + } + + fn insert_parameter( + conn: &mut SqliteConnection, + benchmark_id: BenchmarkId, + parameters: &JsonParameters, + ) -> QueryResult { + diesel::insert_into(schema::parameter::table) + .values(( + schema::parameter::uuid.eq(ParameterUuid::new()), + schema::parameter::benchmark_id.eq(benchmark_id), + schema::parameter::parameters.eq(parameters), + schema::parameter::created.eq(DateTime::TEST), + schema::parameter::modified.eq(DateTime::TEST), + )) + .execute(conn) + } + + fn count_parameters(conn: &mut SqliteConnection, benchmark_id: BenchmarkId) -> i64 { + schema::parameter::table + .filter(schema::parameter::benchmark_id.eq(benchmark_id)) + .count() + .get_result(conn) + .expect("Failed to count parameters") + } + + #[test] + fn key_order_collides_on_unique() { + let mut conn = setup_test_db(); + let base = create_base_entities(&mut conn); + let benchmark_id = create_benchmark( + &mut conn, + base.project_id, + "00000000-0000-0000-0000-000000000010", + "bench1", + "bench1", + ); + + insert_parameter(&mut conn, benchmark_id, ¶meters(r#"{"b": 1, "a": 2}"#)) + .expect("Failed to insert parameter"); + let collision = + insert_parameter(&mut conn, benchmark_id, ¶meters(r#"{"a": 2, "b": 1}"#)); + + assert!( + collision.is_err(), + "logically equal parameter sets must collide on UNIQUE(benchmark_id, parameters)" + ); + // The empty set the benchmark was born with, plus the one that landed. + assert_eq!(count_parameters(&mut conn, benchmark_id), 2); + } + + #[test] + fn number_spelling_collides_on_unique() { + let mut conn = setup_test_db(); + let base = create_base_entities(&mut conn); + let benchmark_id = create_benchmark( + &mut conn, + base.project_id, + "00000000-0000-0000-0000-000000000010", + "bench1", + "bench1", + ); + + insert_parameter(&mut conn, benchmark_id, ¶meters(r#"{"n": 16}"#)) + .expect("Failed to insert parameter"); + for spelling in [r#"{"n": 16.0}"#, r#"{"n": 1.6e1}"#] { + assert!( + insert_parameter(&mut conn, benchmark_id, ¶meters(spelling)).is_err(), + "{spelling} must collide with 16" + ); + } + + assert_eq!(count_parameters(&mut conn, benchmark_id), 2); + } + + #[test] + fn identical_parameters_under_distinct_benchmarks() { + let mut conn = setup_test_db(); + let base = create_base_entities(&mut conn); + let first = create_benchmark( + &mut conn, + base.project_id, + "00000000-0000-0000-0000-000000000010", + "bench1", + "bench1", + ); + let second = create_benchmark( + &mut conn, + base.project_id, + "00000000-0000-0000-0000-000000000011", + "bench2", + "bench2", + ); + + let grid_point = parameters(r#"{"size_mb": 16}"#); + insert_parameter(&mut conn, first, &grid_point).expect("Failed to insert parameter"); + insert_parameter(&mut conn, second, &grid_point).expect("Failed to insert parameter"); + + assert_eq!(count_parameters(&mut conn, first), 2); + assert_eq!(count_parameters(&mut conn, second), 2); + } + + /// Seed the pre-migration shape: benchmarks and `report_benchmark` rows with + /// no `parameter_id`, written as raw SQL because the Diesel DSL describes the + /// post-migration schema. + fn seed_legacy_rows(conn: &mut SqliteConnection) { + conn.batch_execute( + "INSERT INTO organization (uuid, name, slug, created, modified) + VALUES ('00000000-0000-0000-0000-000000000001', 'Org', 'org', 0, 0); + INSERT INTO project (uuid, organization_id, name, slug, visibility, created, modified) + VALUES ('00000000-0000-0000-0000-000000000002', 1, 'Project', 'project', 0, 0, 0); + INSERT INTO branch (uuid, project_id, name, slug, created, modified) + VALUES ('00000000-0000-0000-0000-000000000003', 1, 'main', 'main', 0, 0); + INSERT INTO head (uuid, branch_id, created) + VALUES ('00000000-0000-0000-0000-000000000004', 1, 0); + UPDATE branch SET head_id = 1 WHERE id = 1; + INSERT INTO version (uuid, project_id, number) + VALUES ('00000000-0000-0000-0000-000000000005', 1, 1); + INSERT INTO head_version (head_id, version_id) VALUES (1, 1); + INSERT INTO testbed (uuid, project_id, name, slug, created, modified) + VALUES ('00000000-0000-0000-0000-000000000006', 1, 'localhost', 'localhost', 0, 0); + INSERT INTO report (uuid, project_id, head_id, version_id, testbed_id, adapter, start_time, end_time, created) + VALUES ('00000000-0000-0000-0000-000000000007', 1, 1, 1, 1, 0, 0, 0, 0); + INSERT INTO benchmark (uuid, project_id, name, slug, created, modified) + VALUES ('00000000-0000-0000-0000-000000000008', 1, 'bench1', 'bench1', 0, 0); + INSERT INTO benchmark (uuid, project_id, name, slug, created, modified) + VALUES ('00000000-0000-0000-0000-000000000009', 1, 'bench2', 'bench2', 0, 0); + INSERT INTO report_benchmark (uuid, report_id, iteration, benchmark_id) + VALUES ('00000000-0000-0000-0000-000000000010', 1, 0, 1); + INSERT INTO report_benchmark (uuid, report_id, iteration, benchmark_id) + VALUES ('00000000-0000-0000-0000-000000000011', 1, 1, 1); + INSERT INTO report_benchmark (uuid, report_id, iteration, benchmark_id) + VALUES ('00000000-0000-0000-0000-000000000012', 1, 0, 2);", + ) + .expect("Failed to seed legacy rows"); + } + + #[test] + fn migration_backfills_empty_parameter_sets() { + let mut conn = setup_test_db(); + + // Foreign keys cannot be toggled inside a transaction, and Diesel runs each + // migration in one, so they are disabled around the revert and re-apply. + conn.batch_execute("PRAGMA foreign_keys = OFF") + .expect("Failed to disable foreign keys"); + conn.revert_last_migration(crate::MIGRATIONS) + .expect("Failed to revert the parameter migration"); + + seed_legacy_rows(&mut conn); + + conn.run_pending_migrations(crate::MIGRATIONS) + .expect("Failed to re-apply the parameter migration"); + conn.batch_execute("PRAGMA foreign_keys = ON") + .expect("Failed to enable foreign keys"); + + let benchmark_ids: Vec = schema::benchmark::table + .order(schema::benchmark::id.asc()) + .select(schema::benchmark::id) + .load(&mut conn) + .expect("Failed to load benchmarks"); + assert_eq!(benchmark_ids.len(), 2); + + for benchmark_id in benchmark_ids { + let backfilled: Vec = schema::parameter::table + .filter(schema::parameter::benchmark_id.eq(benchmark_id)) + .select(schema::parameter::parameters) + .load(&mut conn) + .expect("Failed to load parameters"); + assert_eq!( + backfilled, + vec![JsonParameters::default()], + "every benchmark gets exactly one empty parameter set" + ); + + // The migration mints the canonical empty object in SQL, so a set minted + // in Rust has to be byte identical to it. + assert!( + insert_parameter(&mut conn, benchmark_id, &JsonParameters::default()).is_err(), + "the backfilled empty set must collide with a Rust minted one" + ); + } + + let report_benchmarks: Vec<(BenchmarkId, super::ParameterId)> = + schema::report_benchmark::table + .select(( + schema::report_benchmark::benchmark_id, + schema::report_benchmark::parameter_id, + )) + .load(&mut conn) + .expect("Failed to load report benchmarks"); + assert_eq!(report_benchmarks.len(), 3); + for (benchmark_id, parameter_id) in report_benchmarks { + let empty_set_id = super::QueryParameter::get_empty_set_id(&mut conn, benchmark_id) + .expect("Failed to get the empty parameter set"); + assert_eq!( + parameter_id, empty_set_id, + "every report benchmark points at its own benchmark's empty set" + ); + } + } + + #[test] + fn migration_down_and_up_is_idempotent() { + let mut conn = setup_test_db(); + let base = create_base_entities(&mut conn); + let benchmark_id = create_benchmark( + &mut conn, + base.project_id, + "00000000-0000-0000-0000-000000000010", + "bench1", + "bench1", + ); + assert_eq!(count_parameters(&mut conn, benchmark_id), 1); + + conn.batch_execute("PRAGMA foreign_keys = OFF") + .expect("Failed to disable foreign keys"); + conn.revert_last_migration(crate::MIGRATIONS) + .expect("Failed to revert the parameter migration"); + conn.run_pending_migrations(crate::MIGRATIONS) + .expect("Failed to re-apply the parameter migration"); + conn.batch_execute("PRAGMA foreign_keys = ON") + .expect("Failed to enable foreign keys"); + + assert_eq!(count_parameters(&mut conn, benchmark_id), 1); + } +} diff --git a/lib/bencher_schema/src/model/project/report/report_benchmark.rs b/lib/bencher_schema/src/model/project/report/report_benchmark.rs index f029ad8a15..0813994adf 100644 --- a/lib/bencher_schema/src/model/project/report/report_benchmark.rs +++ b/lib/bencher_schema/src/model/project/report/report_benchmark.rs @@ -2,7 +2,7 @@ use bencher_json::{ReportBenchmarkUuid, project::report::Iteration}; use crate::{ macros::fn_get::{fn_get, fn_get_id, fn_get_uuid}, - model::project::benchmark::BenchmarkId, + model::project::{benchmark::BenchmarkId, parameter::ParameterId}, schema::report_benchmark as report_benchmark_table, }; @@ -19,6 +19,7 @@ pub struct QueryReportBenchmark { pub report_id: ReportId, pub iteration: Iteration, pub benchmark_id: BenchmarkId, + pub parameter_id: ParameterId, } impl QueryReportBenchmark { @@ -34,15 +35,159 @@ pub struct InsertReportBenchmark { pub report_id: ReportId, pub iteration: Iteration, pub benchmark_id: BenchmarkId, + pub parameter_id: ParameterId, } impl InsertReportBenchmark { - pub fn from_json(report_id: ReportId, iteration: Iteration, benchmark_id: BenchmarkId) -> Self { + pub fn from_json( + report_id: ReportId, + iteration: Iteration, + benchmark_id: BenchmarkId, + parameter_id: ParameterId, + ) -> Self { InsertReportBenchmark { uuid: ReportBenchmarkUuid::new(), report_id, iteration, benchmark_id, + parameter_id, } } } + +#[cfg(test)] +mod tests { + use bencher_json::{DateTime, JsonParameters, ParameterUuid, ReportBenchmarkUuid}; + use diesel::{ + ExpressionMethods as _, QueryDsl as _, QueryResult, RunQueryDsl as _, SqliteConnection, + }; + + use crate::{ + macros::sql::last_insert_rowid, + model::project::{benchmark::BenchmarkId, parameter::ParameterId, report::ReportId}, + schema, + test_util::{ + create_base_entities, create_benchmark, create_branch_with_head, create_report, + create_testbed, create_version, get_empty_parameter, setup_test_db, + }, + }; + + struct TestRows { + report: ReportId, + benchmark: BenchmarkId, + empty_set: ParameterId, + grid_point: ParameterId, + } + + fn seed(conn: &mut SqliteConnection) -> TestRows { + let base = create_base_entities(conn); + let branch = create_branch_with_head( + conn, + base.project_id, + "00000000-0000-0000-0000-000000000003", + "main", + "main", + "00000000-0000-0000-0000-000000000004", + ); + let version_id = create_version( + conn, + base.project_id, + "00000000-0000-0000-0000-000000000005", + 1, + None, + ); + let testbed_id = create_testbed( + conn, + base.project_id, + "00000000-0000-0000-0000-000000000006", + "localhost", + "localhost", + ); + let report_id = create_report( + conn, + "00000000-0000-0000-0000-000000000007", + base.project_id, + branch.head_id, + version_id, + testbed_id, + ); + let benchmark_id = create_benchmark( + conn, + base.project_id, + "00000000-0000-0000-0000-000000000008", + "bench1", + "bench1", + ); + let empty_set_id = get_empty_parameter(conn, benchmark_id); + + let grid_point: JsonParameters = + r#"{"size_mb": 16}"#.parse().expect("Failed to parse parameters"); + diesel::insert_into(schema::parameter::table) + .values(( + schema::parameter::uuid.eq(ParameterUuid::new()), + schema::parameter::benchmark_id.eq(benchmark_id), + schema::parameter::parameters.eq(&grid_point), + schema::parameter::created.eq(DateTime::TEST), + schema::parameter::modified.eq(DateTime::TEST), + )) + .execute(&mut *conn) + .expect("Failed to insert parameter"); + let grid_point_id: ParameterId = diesel::select(last_insert_rowid()) + .get_result(&mut *conn) + .expect("Failed to get parameter id"); + + TestRows { + report: report_id, + benchmark: benchmark_id, + empty_set: empty_set_id, + grid_point: grid_point_id, + } + } + + fn insert_report_benchmark( + conn: &mut SqliteConnection, + rows: &TestRows, + parameter_id: ParameterId, + ) -> QueryResult { + diesel::insert_into(schema::report_benchmark::table) + .values(( + schema::report_benchmark::uuid.eq(ReportBenchmarkUuid::new()), + schema::report_benchmark::report_id.eq(rows.report), + schema::report_benchmark::iteration.eq(0), + schema::report_benchmark::benchmark_id.eq(rows.benchmark), + schema::report_benchmark::parameter_id.eq(parameter_id), + )) + .execute(conn) + } + + #[test] + fn grid_points_coexist_in_one_iteration() { + let mut conn = setup_test_db(); + let rows = seed(&mut conn); + + insert_report_benchmark(&mut conn, &rows, rows.empty_set) + .expect("Failed to insert the empty set report benchmark"); + insert_report_benchmark(&mut conn, &rows, rows.grid_point) + .expect("Failed to insert the grid point report benchmark"); + + let count: i64 = schema::report_benchmark::table + .filter(schema::report_benchmark::report_id.eq(rows.report)) + .count() + .get_result(&mut conn) + .expect("Failed to count report benchmarks"); + assert_eq!(count, 2); + } + + #[test] + fn same_grid_point_twice_collides() { + let mut conn = setup_test_db(); + let rows = seed(&mut conn); + + insert_report_benchmark(&mut conn, &rows, rows.grid_point) + .expect("Failed to insert the grid point report benchmark"); + assert!( + insert_report_benchmark(&mut conn, &rows, rows.grid_point).is_err(), + "one parameter set cannot appear twice in one report iteration" + ); + } +} diff --git a/lib/bencher_schema/src/model/project/report/results/mod.rs b/lib/bencher_schema/src/model/project/report/results/mod.rs index cc5bac470a..ce36ae757f 100644 --- a/lib/bencher_schema/src/model/project/report/results/mod.rs +++ b/lib/bencher_schema/src/model/project/report/results/mod.rs @@ -29,6 +29,7 @@ use crate::{ branch::{BranchId, head::HeadId}, measure::{MeasureId, QueryMeasure}, metric::InsertMetric, + parameter::{ParameterId, QueryParameter}, report::report_benchmark::{InsertReportBenchmark, ReportBenchmarkId}, testbed::TestbedId, }, @@ -55,6 +56,7 @@ pub struct ReportResults { #[cfg(feature = "plus")] pub series_cache: SeriesCacheContext, pub benchmark_cache: HashMap, + pub parameter_cache: HashMap, pub measure_cache: HashMap, pub detector_cache: HashMap>, } @@ -90,6 +92,7 @@ impl ReportResults { #[cfg(feature = "plus")] series_cache, benchmark_cache: HashMap::new(), + parameter_cache: HashMap::new(), measure_cache: HashMap::new(), detector_cache: HashMap::new(), } @@ -284,9 +287,12 @@ impl ReportResults { // If benchmark name is ignored then strip the special suffix before querying let (benchmark, ignore_benchmark) = strip_ignore_suffix(benchmark); let benchmark_id = self.benchmark_id(context, benchmark).await?; + // Every result in this layer rides its benchmark's empty parameter set. + // Resolved here in Phase 1 so the Phase 2 write transaction stays read free. + let parameter_id = self.parameter_id(context, benchmark_id).await?; let insert_report_benchmark = - InsertReportBenchmark::from_json(self.report_id, iteration, benchmark_id); + InsertReportBenchmark::from_json(self.report_id, iteration, benchmark_id, parameter_id); let mut prepared_metrics = Vec::with_capacity(metrics.inner.len()); for (measure_key, metric) in metrics.inner { @@ -333,6 +339,20 @@ impl ReportResults { }) } + async fn parameter_id( + &mut self, + context: &ApiContext, + benchmark_id: BenchmarkId, + ) -> Result { + Ok(if let Some(id) = self.parameter_cache.get(&benchmark_id) { + *id + } else { + let parameter_id = QueryParameter::get_empty_set_id(auth_conn!(context), benchmark_id)?; + self.parameter_cache.insert(benchmark_id, parameter_id); + parameter_id + }) + } + async fn measure_id( &mut self, context: &ApiContext, diff --git a/lib/bencher_schema/src/schema.rs b/lib/bencher_schema/src/schema.rs index 5a8e968bc2..c0c1a4cfd1 100644 --- a/lib/bencher_schema/src/schema.rs +++ b/lib/bencher_schema/src/schema.rs @@ -173,6 +173,18 @@ diesel::table! { } } +diesel::table! { + parameter (id) { + id -> Integer, + uuid -> Text, + benchmark_id -> Integer, + parameters -> Jsonb, + created -> BigInt, + modified -> BigInt, + archived -> Nullable, + } +} + diesel::table! { plan (id) { id -> Integer, @@ -301,6 +313,7 @@ diesel::table! { report_id -> Integer, iteration -> Integer, benchmark_id -> Integer, + parameter_id -> Integer, } } @@ -471,6 +484,7 @@ diesel::joinable!(metric -> report_benchmark (report_benchmark_id)); diesel::joinable!(metric_count_by_report -> report (report_id)); diesel::joinable!(organization_role -> organization (organization_id)); diesel::joinable!(organization_role -> user (user_id)); +diesel::joinable!(parameter -> benchmark (benchmark_id)); diesel::joinable!(plot -> project (project_id)); diesel::joinable!(plot_benchmark -> benchmark (benchmark_id)); diesel::joinable!(plot_benchmark -> plot (plot_id)); @@ -492,6 +506,7 @@ diesel::joinable!(report -> testbed (testbed_id)); diesel::joinable!(report -> user (user_id)); diesel::joinable!(report -> version (version_id)); diesel::joinable!(report_benchmark -> benchmark (benchmark_id)); +diesel::joinable!(report_benchmark -> parameter (parameter_id)); diesel::joinable!(report_benchmark -> report (report_id)); diesel::joinable!(runner_spec -> runner (runner_id)); diesel::joinable!(runner_spec -> spec (spec_id)); @@ -526,6 +541,7 @@ diesel::allow_tables_to_appear_in_same_query!( model, organization, organization_role, + parameter, plan, plot, plot_benchmark, diff --git a/lib/bencher_schema/src/test_util.rs b/lib/bencher_schema/src/test_util.rs index 4080c3ca51..e07b62fbd3 100644 --- a/lib/bencher_schema/src/test_util.rs +++ b/lib/bencher_schema/src/test_util.rs @@ -10,7 +10,7 @@ //! so no concurrent INSERT can interleave between the INSERT and the `last_insert_rowid()` call. use bencher_json::{ - DateTime, + DateTime, JsonParameters, ParameterUuid, project::{ alert::AlertStatus, boundary::BoundaryLimit, @@ -31,6 +31,7 @@ use crate::{ branch::{BranchId, head::HeadId, head_version::HeadVersionId, version::VersionId}, measure::MeasureId, metric::MetricId, + parameter::ParameterId, plot::PlotId, report::{ReportId, report_benchmark::ReportBenchmarkId}, testbed::TestbedId, @@ -504,7 +505,7 @@ pub fn create_report( .expect("Failed to get report id") } -/// Create a benchmark for testing. +/// Create a benchmark for testing, with the empty parameter set every benchmark is born with. pub fn create_benchmark( conn: &mut SqliteConnection, project_id: ProjectId, @@ -524,18 +525,74 @@ pub fn create_benchmark( .execute(conn) .expect("Failed to insert benchmark"); + let benchmark_id: BenchmarkId = diesel::select(last_insert_rowid()) + .get_result(conn) + .expect("Failed to get benchmark id"); + + create_parameter(conn, benchmark_id, &JsonParameters::default()); + + benchmark_id +} + +/// Create a parameter set for a benchmark. +pub fn create_parameter( + conn: &mut SqliteConnection, + benchmark_id: BenchmarkId, + parameters: &JsonParameters, +) -> ParameterId { + diesel::insert_into(schema::parameter::table) + .values(( + schema::parameter::uuid.eq(ParameterUuid::new()), + schema::parameter::benchmark_id.eq(benchmark_id), + schema::parameter::parameters.eq(parameters), + schema::parameter::created.eq(DateTime::TEST), + schema::parameter::modified.eq(DateTime::TEST), + )) + .execute(conn) + .expect("Failed to insert parameter"); + diesel::select(last_insert_rowid()) .get_result(conn) - .expect("Failed to get benchmark id") + .expect("Failed to get parameter id") +} + +/// Get a benchmark's empty parameter set. +pub fn get_empty_parameter(conn: &mut SqliteConnection, benchmark_id: BenchmarkId) -> ParameterId { + schema::parameter::table + .filter(schema::parameter::benchmark_id.eq(benchmark_id)) + .filter(schema::parameter::parameters.eq(JsonParameters::default())) + .select(schema::parameter::id) + .first(conn) + .expect("Failed to get empty parameter set") } -/// Create a report benchmark for testing. +/// Create a report benchmark for testing, on the benchmark's empty parameter set. pub fn create_report_benchmark( conn: &mut SqliteConnection, report_benchmark_uuid: &str, report_id: ReportId, iteration: i32, benchmark_id: BenchmarkId, +) -> ReportBenchmarkId { + let parameter_id = get_empty_parameter(conn, benchmark_id); + create_report_benchmark_for_parameter( + conn, + report_benchmark_uuid, + report_id, + iteration, + benchmark_id, + parameter_id, + ) +} + +/// Create a report benchmark for testing, on an explicit parameter set. +pub fn create_report_benchmark_for_parameter( + conn: &mut SqliteConnection, + report_benchmark_uuid: &str, + report_id: ReportId, + iteration: i32, + benchmark_id: BenchmarkId, + parameter_id: ParameterId, ) -> ReportBenchmarkId { diesel::insert_into(schema::report_benchmark::table) .values(( @@ -543,6 +600,7 @@ pub fn create_report_benchmark( schema::report_benchmark::report_id.eq(report_id), schema::report_benchmark::iteration.eq(iteration), schema::report_benchmark::benchmark_id.eq(benchmark_id), + schema::report_benchmark::parameter_id.eq(parameter_id), )) .execute(conn) .expect("Failed to insert report_benchmark");