From 54aa7a7a3c557e998ef1dfd48846e4374b6cfce6 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Sat, 15 Aug 2026 16:09:45 +0000 Subject: [PATCH 1/9] Add failing tests for the benchmark parameter dimension Tests first, red before green. The invariants pinned here are: - Every benchmark is born with exactly one empty parameter set, atomically, through both `POST /v0/projects/{project}/benchmarks` and report ingest. - If the parameter set insert fails, the benchmark insert rolls back. - RFC 8785 (JCS) canonicalization: keys sorted by UTF-16 code unit, ECMAScript number formatting, no insignificant whitespace, and `UNIQUE(benchmark_id, parameters)` as the enforcement point. - Parameter values are JSON scalars only; null, arrays, and objects are rejected. - Two parameter sets of one benchmark coexist in a report iteration, and the same parameter set twice collides. - The migration backfills the empty set for every existing benchmark and points every `report_benchmark` row at its own benchmark's empty set. The schema, the migration, and `JsonParameters` ship here as the minimum stub that lets the tests compile: `JsonParameters` wraps a permissive map and `canonical()` is plain `serde_json` serialization, so the canonicalization, key ordering, and scalar validation tests fail on their assertions. `QueryBenchmark::create` does not yet write the empty parameter set, so the birth, atomicity, and ingest tests fail on theirs. --- Cargo.lock | 1 + Cargo.toml | 2 +- lib/api_projects/tests/benchmarks.rs | 138 ++++++- lib/api_projects/tests/metrics.rs | 4 +- lib/api_projects/tests/perf.rs | 13 +- lib/api_projects/tests/reports.rs | 74 +++- lib/bencher_api_tests/src/helpers.rs | 44 ++- lib/bencher_json/src/lib.rs | 1 + lib/bencher_json/src/project/mod.rs | 1 + lib/bencher_json/src/project/parameter.rs | 257 ++++++++++++ .../down.sql | 32 ++ .../up.sql | 72 ++++ lib/bencher_schema/src/error.rs | 2 + lib/bencher_schema/src/model/project/mod.rs | 1 + .../src/model/project/parameter.rs | 367 ++++++++++++++++++ .../model/project/report/report_benchmark.rs | 149 ++++++- .../src/model/project/report/results/mod.rs | 22 +- lib/bencher_schema/src/schema.rs | 16 + lib/bencher_schema/src/test_util.rs | 66 +++- 19 files changed, 1244 insertions(+), 18 deletions(-) create mode 100644 lib/bencher_json/src/project/parameter.rs create mode 100644 lib/bencher_schema/migrations/2026-08-15-120000_benchmark_parameter/down.sql create mode 100644 lib/bencher_schema/migrations/2026-08-15-120000_benchmark_parameter/up.sql create mode 100644 lib/bencher_schema/src/model/project/parameter.rs diff --git a/Cargo.lock b/Cargo.lock index 641c828fec..97c6f8f31e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2763,6 +2763,7 @@ dependencies = [ "downcast-rs", "libsqlite3-sys", "r2d2", + "serde_json", "sqlite-wasm-rs", "time", ] diff --git a/Cargo.toml b/Cargo.toml index 1034867494..6b746d2ea1 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" 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/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_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/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.rs b/lib/bencher_json/src/project/parameter.rs new file mode 100644 index 0000000000..0381bbfc84 --- /dev/null +++ b/lib/bencher_json/src/project/parameter.rs @@ -0,0 +1,257 @@ +use std::{collections::BTreeMap, fmt, str::FromStr}; + +#[cfg(feature = "schema")] +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +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, Serialize, Deserialize)] +#[cfg_attr(feature = "db", derive(diesel::FromSqlRow, diesel::AsExpression))] +#[cfg_attr(feature = "db", diesel(sql_type = diesel::sql_types::Json))] +pub struct JsonParameters(BTreeMap); + +impl JsonParameters { + /// The RFC 8785 (JCS) canonical serialization of this parameter set. + pub fn canonical(&self) -> String { + serde_json::to_string(&self.0).unwrap_or_default() + } + + /// Whether this is the empty parameter set. + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } +} + +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) + } +} + +#[derive(Debug, thiserror::Error)] +pub enum ParametersError { + #[error("Failed to parse benchmark parameters: {0}")] + Json(serde_json::Error), +} + +#[cfg(test)] +mod tests { + use super::JsonParameters; + + fn canonical(parameters: &str) -> String { + parameters + .parse::() + .expect("Failed to parse parameters") + .canonical() + } + + #[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}"); + } + } + + #[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}"#) + ); + } + + #[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()); + } +} + +#[cfg(feature = "db")] +mod db { + use super::JsonParameters; + + impl diesel::serialize::ToSql for JsonParameters + where + DB: diesel::backend::Backend, + for<'a> String: diesel::serialize::ToSql + + Into< as diesel::query_builder::BindCollector<'a, DB>>::Buffer>, + { + fn to_sql<'b>( + &'b self, + out: &mut diesel::serialize::Output<'b, '_, DB>, + ) -> diesel::serialize::Result { + out.set_value(self.canonical()); + Ok(diesel::serialize::IsNull::No) + } + } + + impl diesel::deserialize::FromSql for JsonParameters + where + DB: diesel::backend::Backend, + String: diesel::deserialize::FromSql, + { + fn from_sql(bytes: DB::RawValue<'_>) -> diesel::deserialize::Result { + Ok(String::from_sql(bytes)?.parse()?) + } + } +} 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..e94281c225 --- /dev/null +++ b/lib/bencher_schema/migrations/2026-08-15-120000_benchmark_parameter/up.sql @@ -0,0 +1,72 @@ +PRAGMA foreign_keys = off; +-- parameter +-- `parameters` holds 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 `TEXT` to match the SQLite representation of Diesel's +-- `Json` SQL type; SQLite's JSON functions read canonical JSON text directly. +CREATE TABLE parameter ( + id INTEGER PRIMARY KEY NOT NULL, + uuid TEXT NOT NULL UNIQUE, + benchmark_id INTEGER NOT NULL, + parameters TEXT 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) +); +CREATE INDEX index_parameter_benchmark ON parameter(benchmark_id); +-- 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. +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, + '{}', + 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) +); +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 + INNER JOIN parameter ON ( + parameter.benchmark_id = report_benchmark.benchmark_id + AND parameter.parameters = '{}' + ); +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); +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/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..58ed31d9c6 --- /dev/null +++ b/lib/bencher_schema/src/model/project/parameter.rs @@ -0,0 +1,367 @@ +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 crate::{ + model::project::benchmark::BenchmarkId, + schema, + test_util::{create_base_entities, create_benchmark, setup_test_db}, + }; + + fn parameters(parameters: &str) -> JsonParameters { + parameters.parse().expect("Failed to parse parameters") + } + + 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..1910405474 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 -> Json, + 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"); From 29f00bf35d77c782b9c52ac9f2726f7e1833d027 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Sat, 15 Aug 2026 16:51:09 +0000 Subject: [PATCH 2/9] Implement the benchmark parameter dimension Every benchmark is now created atomically with its empty parameter set, inside `QueryBenchmark::create`'s existing write transaction, and every `report_benchmark` row carries the parameter set its results belong to. The migration backfills the empty set for every existing benchmark and points every existing `report_benchmark` row at its own benchmark's set. `JsonParameters` holds a canonical map of JSON scalars. Canonicalization is RFC 8785 (JCS): keys sorted by UTF-16 code unit, ECMAScript number formatting, and no insignificant whitespace. The canonical form is what is stored, so `UNIQUE(benchmark_id, parameters)` is the enforcement point for canonical equality. Two dependency changes carry that canonical form: - `ryu-js` formats numbers exactly the way ECMAScript `Number::toString` does, including the round to even tie break that a shortest round trip formatter gets wrong. It passes all of RFC 8785 appendix B. - `serde_json`'s `float_roundtrip` makes float parsing correctly rounded. Without it parsing can land a unit in the last place away from the value in the text, which makes a canonical form unstable across a write and a read: the same logical parameter set would take two rows. Eight adapter test values move by at most a unit in the last place as a result, and are updated here. --- Cargo.lock | 7 + Cargo.toml | 7 +- lib/bencher_adapter/src/adapters/java/jmh.rs | 2 +- .../src/adapters/shell/hyperfine.rs | 16 +- lib/bencher_json/Cargo.toml | 1 + lib/bencher_json/src/project/parameter.rs | 321 +++++++++++++++++- .../src/model/project/benchmark.rs | 15 +- 7 files changed, 351 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 97c6f8f31e..81bf8ed873 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1380,6 +1380,7 @@ dependencies = [ "ordered-float", "percent-encoding", "pretty_assertions", + "ryu-js", "schemars", "serde", "serde_json", @@ -7339,6 +7340,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 6b746d2ea1..6e1c0af861 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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/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_json/Cargo.toml b/lib/bencher_json/Cargo.toml index ee0632ea92..5570cab877 100644 --- a/lib/bencher_json/Cargo.toml +++ b/lib/bencher_json/Cargo.toml @@ -24,6 +24,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/project/parameter.rs b/lib/bencher_json/src/project/parameter.rs index 0381bbfc84..80d590cd5b 100644 --- a/lib/bencher_json/src/project/parameter.rs +++ b/lib/bencher_json/src/project/parameter.rs @@ -1,8 +1,9 @@ -use std::{collections::BTreeMap, fmt, str::FromStr}; +use std::{cmp::Ordering, collections::BTreeMap, fmt, str::FromStr}; +use ordered_float::OrderedFloat; #[cfg(feature = "schema")] use schemars::JsonSchema; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize, Serializer, de, ser::SerializeMap as _}; crate::typed_uuid::typed_uuid!(ParameterUuid); @@ -16,15 +17,33 @@ crate::typed_uuid::typed_uuid!(ParameterUuid); /// is the enforcement point for canonical equality. /// /// [jcs]: https://www.rfc-editor.org/rfc/rfc8785 -#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +#[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::Json))] -pub struct JsonParameters(BTreeMap); +pub struct JsonParameters(BTreeMap); impl JsonParameters { /// The RFC 8785 (JCS) canonical serialization of this parameter set. pub fn canonical(&self) -> String { - serde_json::to_string(&self.0).unwrap_or_default() + 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. @@ -47,15 +66,227 @@ impl FromStr for JsonParameters { } } +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('"'); + 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), + } + } + canonical.push('"'); +} + +fn hex_digit(nibble: u32) -> char { + char::from_digit(nibble, 16).unwrap_or('0') +} + #[cfg(test)] mod tests { - use super::JsonParameters; + use ordered_float::OrderedFloat; + + use super::{JsonParameters, ParameterKey, ParameterValue}; fn canonical(parameters: &str) -> String { parameters @@ -64,6 +295,19 @@ mod tests { .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("{}"), "{}"); @@ -169,6 +413,71 @@ mod tests { } } + // 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}"#); 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)) From e498da299ab2c7fa0a2d39a68ce1d35079915c92 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Sat, 15 Aug 2026 17:55:28 +0000 Subject: [PATCH 3/9] Index the report benchmark parameter dimension and seed plot fixtures Deleting a benchmark cascades to its parameter sets, so SQLite has to verify that no `report_benchmark` row still references each deleted parameter set. Without an index on `report_benchmark(parameter_id)` that check is a full table scan of `report_benchmark` per deleted parameter set, which makes a project delete quadratic. Add the index alongside the existing `index_report_benchmark_benchmark_report` recreate. Backfill the recreated `report_benchmark` with a `LEFT JOIN` rather than an `INNER JOIN`. Every benchmark is backfilled with an empty parameter set immediately above, so the join can only miss on data that the foreign key already forbids; a `LEFT JOIN` turns that impossible case into a `NOT NULL` violation that fails the migration instead of silently dropping the row. Create the empty parameter set for the benchmark seeded by `seed_plot_dimensions`, the last test fixture that inserted a benchmark directly without one. The birth invariant now holds in the tests exactly as it does in production. --- lib/api_projects/tests/projects.rs | 10 ++++++++-- .../2026-08-15-120000_benchmark_parameter/up.sql | 11 ++++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) 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/bencher_schema/migrations/2026-08-15-120000_benchmark_parameter/up.sql b/lib/bencher_schema/migrations/2026-08-15-120000_benchmark_parameter/up.sql index e94281c225..b5f5969fa9 100644 --- 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 @@ -46,6 +46,10 @@ CREATE TABLE up_report_benchmark ( 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, @@ -61,7 +65,7 @@ SELECT report_benchmark.id, report_benchmark.benchmark_id, parameter.id FROM report_benchmark - INNER JOIN parameter ON ( + LEFT JOIN parameter ON ( parameter.benchmark_id = report_benchmark.benchmark_id AND parameter.parameters = '{}' ); @@ -69,4 +73,9 @@ 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; From b7d672bcceef33305245429d3978afad57e64fa8 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Sun, 16 Aug 2026 04:10:35 +0000 Subject: [PATCH 4/9] Add failing tests for JSONB byte agreement with SQLite The parameter set encoder has to produce the same bytes as SQLite's own jsonb() over the same canonical text, because UNIQUE(benchmark_id, parameters) compares bytes and both writers reach that column. The conformance table covers 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. Each case asserts byte equality with jsonb(), that SQLite's JSON functions accept the bytes, that json() returns the canonical text unchanged, that both writers collide on the unique constraint, and that the set reads back as it was written. bencher_schema now links the bundled SQLite amalgamation, since the migrations it owns call jsonb(), which needs SQLite 3.45 or later. --- Cargo.lock | 1 + .../src/project/parameter/jsonb.rs | 48 +++ .../{parameter.rs => parameter/mod.rs} | 3 + lib/bencher_schema/Cargo.toml | 4 + lib/bencher_schema/src/lib.rs | 2 + .../src/model/project/parameter.rs | 330 +++++++++++++++++- 6 files changed, 387 insertions(+), 1 deletion(-) create mode 100644 lib/bencher_json/src/project/parameter/jsonb.rs rename lib/bencher_json/src/project/{parameter.rs => parameter/mod.rs} (99%) diff --git a/Cargo.lock b/Cargo.lock index 81bf8ed873..73bbb638e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1650,6 +1650,7 @@ dependencies = [ "diesel_migrations", "dropshot", "http 1.4.1", + "libsqlite3-sys", "mail-send", "oso", "pretty_assertions", 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..7a1af25ba2 --- /dev/null +++ b/lib/bencher_json/src/project/parameter/jsonb.rs @@ -0,0 +1,48 @@ +//! SQLite's 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. + +/// A JSONB object under construction. +#[derive(Debug, Default)] +pub struct Object(Vec); + +impl Object { + /// Append a `null` member. + pub fn insert_null(&mut self, _key: &str) -> Result<(), JsonbError> { + Err(JsonbError::Unimplemented) + } + + /// Append a boolean member. + pub fn insert_bool(&mut self, _key: &str, _value: bool) -> Result<(), JsonbError> { + Err(JsonbError::Unimplemented) + } + + /// Append a number member, whose payload is the canonical number text. + pub fn insert_number(&mut self, _key: &str, _canonical: &str) -> Result<(), JsonbError> { + Err(JsonbError::Unimplemented) + } + + /// Append a string member. + pub fn insert_string(&mut self, _key: &str, _value: &str) -> Result<(), JsonbError> { + Err(JsonbError::Unimplemented) + } + + /// The JSONB encoding of the object. + pub fn into_blob(self) -> Result, JsonbError> { + Err(JsonbError::Unimplemented) + } +} + +/// Render a JSONB blob as JSON text. +pub fn to_json(_blob: &[u8]) -> Result { + Err(JsonbError::Unimplemented) +} + +#[derive(Debug, thiserror::Error)] +pub enum JsonbError { + #[error("The JSONB codec is not implemented yet")] + Unimplemented, +} diff --git a/lib/bencher_json/src/project/parameter.rs b/lib/bencher_json/src/project/parameter/mod.rs similarity index 99% rename from lib/bencher_json/src/project/parameter.rs rename to lib/bencher_json/src/project/parameter/mod.rs index 80d590cd5b..4407f82ec0 100644 --- a/lib/bencher_json/src/project/parameter.rs +++ b/lib/bencher_json/src/project/parameter/mod.rs @@ -5,6 +5,9 @@ use ordered_float::OrderedFloat; 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. 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/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/parameter.rs b/lib/bencher_schema/src/model/project/parameter.rs index 58ed31d9c6..e20760714d 100644 --- a/lib/bencher_schema/src/model/project/parameter.rs +++ b/lib/bencher_schema/src/model/project/parameter.rs @@ -131,16 +131,344 @@ mod tests { }; 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, setup_test_db}, + 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 { + blob.iter().map(|byte| format!("{byte:02X}")).collect() + } + + /// 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 + } + + /// 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. + 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" + ); + }, + } + } + } + + // 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, From 4669944023a40dc4966a74e34d84f10367bb36ac Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Sun, 16 Aug 2026 04:13:54 +0000 Subject: [PATCH 5/9] Encode benchmark parameters as SQLite JSONB The parameters column becomes JSONB, encoded by JsonParameters itself. The encoder walks the RFC 8785 canonical form straight into SQLite's binary JSON, matching what SQLite's jsonb() produces over the same text: TEXT until a string needs a JSON escape and TEXTJ once it does, INT or FLOAT chosen by the spelling of the canonical number rather than its magnitude, member order left exactly as canonicalization produced it, and the most compact payload size form. The decoder reads back both writers, since the migration mints the empty set with jsonb(). The impls are written against the SQLite backend rather than being generic: every other backend spells Jsonb differently, and a generic impl would be claiming an encoding it does not have. That is why the db feature now enables diesel/sqlite. --- lib/bencher_json/Cargo.toml | 4 +- .../src/project/parameter/jsonb.rs | 388 +++++++++++++++++- lib/bencher_json/src/project/parameter/mod.rs | 59 ++- .../up.sql | 17 +- .../src/model/project/parameter.rs | 15 +- lib/bencher_schema/src/schema.rs | 2 +- 6 files changed, 437 insertions(+), 48 deletions(-) diff --git a/lib/bencher_json/Cargo.toml b/lib/bencher_json/Cargo.toml index 5570cab877..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"] diff --git a/lib/bencher_json/src/project/parameter/jsonb.rs b/lib/bencher_json/src/project/parameter/jsonb.rs index 7a1af25ba2..0e0740ce8b 100644 --- a/lib/bencher_json/src/project/parameter/jsonb.rs +++ b/lib/bencher_json/src/project/parameter/jsonb.rs @@ -1,48 +1,402 @@ -//! SQLite's JSONB binary encoding. +//! `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. +//! `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. +//! 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> { - Err(JsonbError::Unimplemented) + 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> { - Err(JsonbError::Unimplemented) + pub fn insert_bool(&mut self, key: &str, value: bool) -> Result<(), JsonbError> { + self.insert(key, if value { TRUE } else { FALSE }, &[]) } - /// Append a number member, whose payload is the canonical number text. - pub fn insert_number(&mut self, _key: &str, _canonical: &str) -> Result<(), JsonbError> { - Err(JsonbError::Unimplemented) + /// 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> { - Err(JsonbError::Unimplemented) + 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> { - Err(JsonbError::Unimplemented) + 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. +#[expect( + clippy::cast_possible_truncation, + reason = "the size is at most the inline maximum" +)] +const fn size_nibble(size: u32) -> u8 { + ((size & 0x0f) << 4) as u8 +} + +/// 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. -pub fn to_json(_blob: &[u8]) -> Result { - Err(JsonbError::Unimplemented) +/// +/// 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("The JSONB codec is not implemented yet")] - Unimplemented, + #[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 index 4407f82ec0..a93dc5c331 100644 --- a/lib/bencher_json/src/project/parameter/mod.rs +++ b/lib/bencher_json/src/project/parameter/mod.rs @@ -22,7 +22,7 @@ crate::typed_uuid::typed_uuid!(ParameterUuid); /// [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::Json))] +#[cfg_attr(feature = "db", diesel(sql_type = diesel::sql_types::Jsonb))] pub struct JsonParameters(BTreeMap); impl JsonParameters { @@ -53,6 +53,27 @@ impl JsonParameters { 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 { @@ -260,6 +281,12 @@ impl de::Visitor<'_> for ParameterValueVisitor { /// 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("\\\""), @@ -278,7 +305,6 @@ fn write_json_string(string: &str, canonical: &mut String) { character => canonical.push(character), } } - canonical.push('"'); } fn hex_digit(nibble: u32) -> char { @@ -538,32 +564,31 @@ mod tests { } } +/// 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; + use super::{JsonParameters, jsonb}; - impl diesel::serialize::ToSql for JsonParameters - where - DB: diesel::backend::Backend, - for<'a> String: diesel::serialize::ToSql - + Into< as diesel::query_builder::BindCollector<'a, DB>>::Buffer>, - { + impl diesel::serialize::ToSql for JsonParameters { fn to_sql<'b>( &'b self, - out: &mut diesel::serialize::Output<'b, '_, DB>, + out: &mut diesel::serialize::Output<'b, '_, diesel::sqlite::Sqlite>, ) -> diesel::serialize::Result { - out.set_value(self.canonical()); + out.set_value(self.to_jsonb()?); Ok(diesel::serialize::IsNull::No) } } - impl diesel::deserialize::FromSql for JsonParameters - where - DB: diesel::backend::Backend, - String: diesel::deserialize::FromSql, + impl diesel::deserialize::FromSql + for JsonParameters { - fn from_sql(bytes: DB::RawValue<'_>) -> diesel::deserialize::Result { - Ok(String::from_sql(bytes)?.parse()?) + 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/migrations/2026-08-15-120000_benchmark_parameter/up.sql b/lib/bencher_schema/migrations/2026-08-15-120000_benchmark_parameter/up.sql index b5f5969fa9..ad13282217 100644 --- 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 @@ -1,14 +1,15 @@ PRAGMA foreign_keys = off; -- parameter --- `parameters` holds 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 `TEXT` to match the SQLite representation of Diesel's --- `Json` SQL type; SQLite's JSON functions read canonical JSON text directly. +-- `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 TEXT NOT NULL, + parameters BLOB NOT NULL, created BIGINT NOT NULL, modified BIGINT NOT NULL, archived BIGINT, @@ -22,12 +23,14 @@ CREATE INDEX index_parameter_benchmark ON parameter(benchmark_id); -- 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; @@ -67,7 +70,7 @@ SELECT report_benchmark.id, FROM report_benchmark LEFT JOIN parameter ON ( parameter.benchmark_id = report_benchmark.benchmark_id - AND parameter.parameters = '{}' + AND parameter.parameters = jsonb('{}') ); DROP TABLE report_benchmark; ALTER TABLE up_report_benchmark diff --git a/lib/bencher_schema/src/model/project/parameter.rs b/lib/bencher_schema/src/model/project/parameter.rs index e20760714d..819f0f4a46 100644 --- a/lib/bencher_schema/src/model/project/parameter.rs +++ b/lib/bencher_schema/src/model/project/parameter.rs @@ -148,11 +148,11 @@ mod tests { /// 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. + /// 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()`. + /// 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 @@ -214,10 +214,15 @@ mod tests { } fn hex(blob: &[u8]) -> String { - blob.iter().map(|byte| format!("{byte:02X}")).collect() + 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. + /// 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) @@ -251,7 +256,7 @@ mod tests { .value } - /// Mint a parameter set with SQLite's `jsonb()`, the migration's write path. + /// Mint a parameter set with `SQLite`'s `jsonb()`, the migration's write path. fn mint_parameter( conn: &mut SqliteConnection, benchmark_id: BenchmarkId, diff --git a/lib/bencher_schema/src/schema.rs b/lib/bencher_schema/src/schema.rs index 1910405474..c0c1a4cfd1 100644 --- a/lib/bencher_schema/src/schema.rs +++ b/lib/bencher_schema/src/schema.rs @@ -178,7 +178,7 @@ diesel::table! { id -> Integer, uuid -> Text, benchmark_id -> Integer, - parameters -> Json, + parameters -> Jsonb, created -> BigInt, modified -> BigInt, archived -> Nullable, From e41f868123952c1a672fb1bc99ae591d39cfc504 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Sun, 16 Aug 2026 05:29:59 +0000 Subject: [PATCH 6/9] Assert SQL side legibility for the directly encoded parameter set The null conformance case never reaches the column, because scalar only validation rejects a null parameter value, so its bytes are encoded directly. Byte agreement with jsonb() was the only thing asserted over them. Bind the blob and assert json_valid() and json() as well, which is what every other case gets and what neither needs a column for. --- .../src/model/project/parameter.rs | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/lib/bencher_schema/src/model/project/parameter.rs b/lib/bencher_schema/src/model/project/parameter.rs index 819f0f4a46..ee436062d5 100644 --- a/lib/bencher_schema/src/model/project/parameter.rs +++ b/lib/bencher_schema/src/model/project/parameter.rs @@ -256,6 +256,24 @@ mod tests { .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, @@ -351,6 +369,10 @@ mod tests { 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") @@ -361,6 +383,16 @@ mod tests { 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" + ); }, } } From 5e3327b16888a33c2c44135dfc195d9877d70f58 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Tue, 18 Aug 2026 03:08:11 +0000 Subject: [PATCH 7/9] Drop the redundant parameter benchmark index UNIQUE(benchmark_id, parameters) already builds an index whose leftmost column is benchmark_id, so a separate index on parameter(benchmark_id) serves no lookup the autoindex does not already serve, while costing a second index write on every parameter set insert. This is the same redundancy that 2026-07-07-120000_report_benchmark_index_cleanup removed, where index_report_benchmark duplicated the UNIQUE(report_id, iteration, benchmark_id) autoindex prefix. --- .../migrations/2026-08-15-120000_benchmark_parameter/up.sql | 1 - 1 file changed, 1 deletion(-) 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 index ad13282217..613ac53a85 100644 --- 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 @@ -16,7 +16,6 @@ CREATE TABLE parameter ( FOREIGN KEY (benchmark_id) REFERENCES benchmark (id) ON DELETE CASCADE, UNIQUE(benchmark_id, parameters) ); -CREATE INDEX index_parameter_benchmark ON parameter(benchmark_id); -- 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`: From 321c7a6c813175a932e6409d1798fb69c0ed3420 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Tue, 18 Aug 2026 03:08:17 +0000 Subject: [PATCH 8/9] Encode the payload size nibble without a truncating cast Production code carries no lint suppression without approval, and the size nibble was the one place in the encoder that did. Masking before the cast rather than after it puts the shift in the u8 domain, where a nibble cannot outgrow the byte it moves into, so the cast truncates nothing and needs no telling. Both spellings reduce to (size << 4) & 0xf0, so no encoded byte moves and the jsonb() conformance table passes as it stands. --- lib/bencher_json/src/project/parameter/jsonb.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/bencher_json/src/project/parameter/jsonb.rs b/lib/bencher_json/src/project/parameter/jsonb.rs index 0e0740ce8b..2efcb5b824 100644 --- a/lib/bencher_json/src/project/parameter/jsonb.rs +++ b/lib/bencher_json/src/project/parameter/jsonb.rs @@ -150,12 +150,12 @@ fn push_element(blob: &mut Vec, element: u8, payload: &[u8]) -> Result<(), J } /// A payload size small enough to ride in the header's high nibble. -#[expect( - clippy::cast_possible_truncation, - reason = "the size is at most the inline maximum" -)] +/// +/// 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) << 4) as u8 + ((size & 0x0f) as u8) << 4 } /// One byte of a big endian payload size. From 54e67e65acdeb9912f2a040ede22bd9b18b0d601 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Sat, 22 Aug 2026 13:17:57 +0000 Subject: [PATCH 9/9] Pin duplicate parameter keys as last wins A parameter set that names the same key twice is not an error. The later value wins, which is what parsing into a map already does and what the JSON parser any harness is likely to reach for already does. That is the decided behavior rather than an accident of the implementation: it is the least bad of the options, it is the easiest to explain, and it just works. 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, so both survive canonicalization. Nothing moves. The test pins what the deserializer already does, so that a later rewrite of it cannot quietly drop a key or start rejecting a payload that used to be accepted. --- lib/bencher_json/src/project/parameter/mod.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/bencher_json/src/project/parameter/mod.rs b/lib/bencher_json/src/project/parameter/mod.rs index a93dc5c331..596cc9f904 100644 --- a/lib/bencher_json/src/project/parameter/mod.rs +++ b/lib/bencher_json/src/project/parameter/mod.rs @@ -523,6 +523,20 @@ mod tests { ); } + // 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}"#);