From 2f2d23191fe22028c00e9e80393596fadeac3c00 Mon Sep 17 00:00:00 2001 From: Alexander Schrab Date: Sun, 25 Oct 2026 22:00:04 +0100 Subject: [PATCH] perf: keep each surface's boundary curves for CoincidenceCheck (outputs bit-identical) Not upstream: SmSurface::CoincidenceCheck makes both surfaces' natural boundary curves again for every check, and a boolean runs it for every face pair. Each Surface value now keeps them in a memo (BoundaryMemo, a OnceLock), keyed by the domain's bits and the kernel inputs of the build. The check borrows the curves, and the curves carry their curve cache memos, so each boundary cache is built once per surface value. Only B-spline, plane, cone, sphere and torus surfaces are memoised. A copy starts with an empty memo; an edit through &mut Surface replaces it. Kernel::memoize_curve_caches and the curve-memo-off feature turn it off with the curve cache memo. #948 loft, 20 sections: 128 curve-cache builds in CoincidenceCheck (C++ 600), 937 in the whole run (C++ 1393). Whole-run instructions: 57.3 G -> 41.9 G at 20 sections, 179.1 G -> 107.3 G at 40 (C++ 41.7 G, 127.8 G). Co-Authored-By: Claude Opus 5.5 --- crates/solidcore/src/curve/curve/p5.rs | 9 + crates/solidcore/src/kernel.rs | 6 +- crates/solidcore/src/surface/surface/p2.rs | 21 ++ crates/solidcore/src/surface/surface/p8.rs | 185 +++++++++++++++++- crates/solidcore/tests/curve_cache_memo.rs | 15 +- ...e.surface_p8--coincidence-boundary-memo.md | 31 +++ 6 files changed, 262 insertions(+), 5 deletions(-) create mode 100644 plan/findings/surface.surface_p8--coincidence-boundary-memo.md diff --git a/crates/solidcore/src/curve/curve/p5.rs b/crates/solidcore/src/curve/curve/p5.rs index 326b2a39f6..45a82cfa1a 100644 --- a/crates/solidcore/src/curve/curve/p5.rs +++ b/crates/solidcore/src/curve/curve/p5.rs @@ -325,6 +325,15 @@ impl CurveMemoKey { if !k.memoize_curve_caches || !memo_pure(curve) { return None; } + Self::of_kernel(k) + } + + /// The kernel part of a memo key, for any memo of a build that reads `k` only + /// through its tolerances and context scalars; `None` when the memo is off. + pub(crate) fn of_kernel(k: &Kernel) -> Option { + if !k.memoize_curve_caches { + return None; + } let c = &k.context; Some(Self { tolerances: k.tolerances, diff --git a/crates/solidcore/src/kernel.rs b/crates/solidcore/src/kernel.rs index 5f6b30d24e..40ad3367ca 100644 --- a/crates/solidcore/src/kernel.rs +++ b/crates/solidcore/src/kernel.rs @@ -194,8 +194,10 @@ pub struct Kernel { /// [`Cell`] because the intersectors take `&Kernel`. pub cone_parallel_midpoint_test: Cell, /// Not upstream: whether `make_curve_cache` keeps a memo of each curve cache in the - /// curve value (`true` unless the `curve-memo-off` feature is on). The memo never - /// changes a result, only the time; `false` builds every cache fresh, to check that. + /// curve value, and `CoincidenceCheck` a memo of each surface's boundary curves in + /// the surface value (`true` unless the `curve-memo-off` feature is on). The memos + /// never change a result, only the time; `false` builds everything fresh, to check + /// that. pub memoize_curve_caches: bool, } diff --git a/crates/solidcore/src/surface/surface/p2.rs b/crates/solidcore/src/surface/surface/p2.rs index 825f0b63df..37f3eab583 100644 --- a/crates/solidcore/src/surface/surface/p2.rs +++ b/crates/solidcore/src/surface/surface/p2.rs @@ -233,6 +233,7 @@ use crate::surface::surf_of_extrusion::SurfOfExtrusion; use crate::surface::surf_of_revolution::p3::intersect_revolved; use crate::surface::surf_of_revolution::{Revolved, SurfOfRevolution}; use crate::surface::surface::p1::{StepSurface, SurfaceIntersectContext, SurfaceSolveContext}; +use crate::surface::surface::p8::BoundaryMemo; use crate::surface::surface::p8::FaceSolveCache; use crate::surface::surface_cache::p1::SurfaceCache; use crate::surface::torus::Torus; @@ -352,6 +353,9 @@ pub struct Surface { /// The identity a context issued this surface object for its notifications /// (upstream passes the object itself, known by address); a copy has none. notify_identity: IdentitySlot, + /// Not upstream: the memo of the natural boundary curves `CoincidenceCheck` makes + /// (`BoundaryMemo`); a copy starts an empty one, an edit replaces it. + boundary_memo: BoundaryMemo, } /// What a surface's cache slot (`SmAObject::m_pCacheObj`) holds, as far as a reader of @@ -454,6 +458,7 @@ impl Clone for Surface { owner_zone_tol_3d: None, cache_slot: CacheSlot::default(), notify_identity: IdentitySlot::new(), + boundary_memo: BoundaryMemo::new(), }; s.update_owned_bases(); s @@ -694,6 +699,7 @@ impl Surface { owner_zone_tol_3d: self.owner_zone_tol_3d, cache_slot: self.cache_slot.share(), notify_identity: IdentitySlot::new(), + boundary_memo: BoundaryMemo::new(), }) } @@ -740,6 +746,7 @@ impl Surface { owner_zone_tol_3d: self.owner_zone_tol_3d, cache_slot: self.cache_slot.share(), notify_identity: IdentitySlot::new(), + boundary_memo: BoundaryMemo::new(), } } } @@ -890,6 +897,7 @@ impl Surface { owner_zone_tol_3d: None, cache_slot: CacheSlot::default(), notify_identity: IdentitySlot::new(), + boundary_memo: BoundaryMemo::new(), } } @@ -1087,6 +1095,11 @@ impl Surface { self.cache_slot.same(&other.cache_slot) } + /// The boundary curve memo of `CoincidenceCheck` (see `BoundaryMemo`). + pub(crate) const fn boundary_memo(&self) -> &BoundaryMemo { + &self.boundary_memo + } + /// The concrete kind. #[must_use] pub const fn kind(&self) -> &SurfaceKind { @@ -1095,6 +1108,8 @@ impl Surface { /// The concrete kind, mutable. pub fn kind_mut(&mut self) -> &mut SurfaceKind { + // an edit makes another surface value: start an empty boundary memo + self.boundary_memo = BoundaryMemo::new(); &mut self.kind } @@ -2059,6 +2074,7 @@ impl Surface { k: &Kernel, new_step_uv_domain: &Extent2d, ) -> Result<()> { + self.boundary_memo = BoundaryMemo::new(); match &mut self.kind { SurfaceKind::BSpline(s) => s.adjust_step_uv_domain(new_step_uv_domain), SurfaceKind::CurveBounded(_) @@ -2082,6 +2098,7 @@ impl Surface { /// /// Those of the kind's override (the base default is `SM_ERR`). pub fn reverse(&mut self, k: &Kernel, param: SurfParamType) -> Result<()> { + self.boundary_memo = BoundaryMemo::new(); match &mut self.kind { SurfaceKind::BSpline(s) => s.reverse(param), SurfaceKind::CurveBounded(_) | SurfaceKind::Step(_) => base_edit(), @@ -2144,6 +2161,7 @@ impl Surface { /// /// Those of the kind's override (the base default is `SM_ERR`). pub fn reparameterize(&mut self, k: &Kernel, new_domain: &Extent2d) -> Result<()> { + self.boundary_memo = BoundaryMemo::new(); match &mut self.kind { SurfaceKind::BSpline(s) => s.reparameterize(new_domain), SurfaceKind::CurveBounded(_) @@ -2168,6 +2186,7 @@ impl Surface { /// /// Those of the kind's override (the base default is `SM_ERR`). pub fn swap_uv(&mut self, k: &Kernel) -> Result<()> { + self.boundary_memo = BoundaryMemo::new(); match &mut self.kind { SurfaceKind::BSpline(s) => s.swap_uv(), SurfaceKind::CurveBounded(_) | SurfaceKind::Step(_) => base_edit(), @@ -2243,6 +2262,7 @@ impl Surface { scale: Option, depth: usize, ) -> Result<()> { + self.boundary_memo = BoundaryMemo::new(); match &mut self.kind { SurfaceKind::Cone(s) => s.transform(k, rotate_n_move, scale), SurfaceKind::Sphere(s) => s.transform(k, rotate_n_move, scale), @@ -2282,6 +2302,7 @@ impl Surface { trim_domain: &mut Extent2d, options: TrimOptions, ) -> Result<()> { + self.boundary_memo = BoundaryMemo::new(); match &mut self.kind { SurfaceKind::BSpline(s) => s.trim_with_domain_with(trim_domain, options), SurfaceKind::CurveBounded(_) | SurfaceKind::Step(_) => base_edit(), diff --git a/crates/solidcore/src/surface/surface/p8.rs b/crates/solidcore/src/surface/surface/p8.rs index a309cffdef..e88f425a74 100644 --- a/crates/solidcore/src/surface/surface/p8.rs +++ b/crates/solidcore/src/surface/surface/p8.rs @@ -16,6 +16,12 @@ //! | [`extract_iso_curve`] | `sm_ExtractIsoCurve` (static) | //! | [`intersect_srf_bndry_crv`] | `sm_IntersectSrfBndryCrv` (static) | //! +//! Deviations (not upstream, beyond the port): `CoincidenceCheck` keeps the surfaces' +//! natural boundary curves in a memo in each surface value (`BoundaryMemo`), where +//! upstream makes them again for every check. The curves are a function of the surface +//! value, the domain and the kernel's tolerances, so a hit gives what a fresh build +//! gives; `Kernel::memoize_curve_caches` turns it off with the curve cache memo. +//! //! Deviations: out-parameters become return values. The caller's object handles //! (`O`, the objects named in solutions) replace the `this` and `&crCurve` pointers //! upstream stores in them. Solvers read the patches of the surface cache they were @@ -64,6 +70,7 @@ use super::p2::{ use super::p4::{is_point_on_normal, NoObject}; use super::p5::DropPointOptions; use super::p6::SampleOptions; +use crate::curve::curve::p5::CurveMemoKey; use crate::curve::curve::{Curve, CurveKind}; use crate::curve::curve_cache::CurveCache; use crate::curve::line::Line; @@ -96,6 +103,8 @@ use crate::surface::plane::Coincidence; use crate::surface::surface::p7::SpGlobalSolver; use crate::surface::surface_cache::p1::SurfaceCache; use crate::surface::surface_cache::BezierPatch; +use std::borrow::Cow; +use std::sync::OnceLock; /// A surface's solve cache kept by a caller across [`Surface::global_line_intersect_held`] /// calls (upstream keeps one cache on each surface): `None` until the first call @@ -304,6 +313,104 @@ fn b_spline_part(curve: &Curve) -> Option<&BSplineCurve> { /// The 3D side curves of the natural UV trim loop of `surface` over `domain` /// (`CreateNaturalUVTrimCurves(ctx, domain, SM_SP_V, FALSE, ..)`); the status is /// not checked upstream. +/// The key of a memoised [`trim_curves`] result (not upstream): the kernel inputs of the +/// build ([`CurveMemoKey::of_kernel`]) and the domain's bits. +#[derive(Debug, Clone, Copy, PartialEq)] +struct BoundaryMemoKey { + kernel: CurveMemoKey, + domain: [u64; 4], +} + +impl BoundaryMemoKey { + /// The key of `trim_curves(k, surface, domain)`, or `None` when it must not be + /// memoised: the memo is off, or the surface is not a pure value. B-spline, plane, + /// cone, sphere and torus surfaces build their sides from their own value (their + /// `CreateIsoParametricCurve` overrides read nothing else); the other kinds hold + /// curves, other surfaces or volumes, or approximate through the base body. + fn of(k: &Kernel, surface: &Surface, domain: &Extent2d) -> Option { + let pure_kind = matches!( + surface.kind(), + SurfaceKind::BSpline(_) + | SurfaceKind::Plane(_) + | SurfaceKind::Cone(_) + | SurfaceKind::Sphere(_) + | SurfaceKind::Torus(_) + ); + if !pure_kind { + return None; + } + Some(Self { + kernel: CurveMemoKey::of_kernel(k)?, + domain: [ + domain.min().x.to_bits(), + domain.min().y.to_bits(), + domain.max().x.to_bits(), + domain.max().y.to_bits(), + ], + }) + } +} + +/// One memoised [`trim_curves`] result. +#[derive(Debug)] +struct BoundaryMemoEntry { + key: BoundaryMemoKey, + curves: Vec>, +} + +/// The memo of a surface's natural boundary curves for `CoincidenceCheck` (not +/// upstream: upstream makes the four curves again for every check, here every face +/// pair of a boolean). It lives in the [`Surface`] value and is filled once; a copy +/// starts an empty one and an edit through `&mut Surface` replaces it. The curves are a +/// function of the surface value and the key, so a hit gives what a fresh build gives, +/// and the curves carry their own curve cache memos, so their caches are built once +/// too. Lent by reference: the check only reads the curves. One entry per live surface, +/// freed with it; no `unsafe`, and the surface stays `Send`. +#[derive(Default)] +pub(crate) struct BoundaryMemo(OnceLock); + +impl core::fmt::Debug for BoundaryMemo { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + // the memo is not part of the surface's observable state + f.write_str("BoundaryMemo") + } +} + +impl BoundaryMemo { + /// An empty memo. + pub(crate) const fn new() -> Self { + Self(OnceLock::new()) + } +} + +/// [`trim_curves`] through the surface's [`BoundaryMemo`]: borrowed from the memo on a +/// hit (or after filling it), built and owned when the key differs or the surface is +/// not memoised. +fn boundary_curves<'s>( + k: &Kernel, + surface: &'s Surface, + domain: &Extent2d, +) -> Result]>> { + let Some(key) = BoundaryMemoKey::of(k, surface, domain) else { + return trim_curves(k, surface, domain).map(Cow::Owned); + }; + let memo = &surface.boundary_memo().0; + if let Some(entry) = memo.get() { + if entry.key == key { + return Ok(Cow::Borrowed(&entry.curves)); + } + return trim_curves(k, surface, domain).map(Cow::Owned); + } + let curves = trim_curves(k, surface, domain)?; + if let Err(entry) = memo.set(BoundaryMemoEntry { key, curves }) { + return Ok(Cow::Owned(entry.curves)); + } + match memo.get() { + Some(entry) => Ok(Cow::Borrowed(&entry.curves)), + None => trim_curves(k, surface, domain).map(Cow::Owned), + } +} + fn trim_curves(k: &Kernel, surface: &Surface, domain: &Extent2d) -> Result>> { let t = ignored(surface.create_natural_uv_trim_curves(k, domain, SurfParamType::V, false))?; Ok(t.map(|t| t.curves_3d.into_iter().map(Some).collect()) @@ -480,8 +587,11 @@ impl Surface { // Now we're looking for any partial overlap. // Create two sets of boundary curves. - let mut bdrys0 = trim_curves(k, self, &domain0)?; - let mut bdrys1 = trim_curves(k, other, &domain1)?; + // (not upstream: kept on the surfaces across checks, see BoundaryMemo) + let all0 = boundary_curves(k, self, &domain0)?; + let all1 = boundary_curves(k, other, &domain1)?; + let mut bdrys0: Vec> = all0.iter().map(Option::as_ref).collect(); + let mut bdrys1: Vec> = all1.iter().map(Option::as_ref).collect(); // We check angles a lot, so get a good angular tolerance. let bbox = bbox0.union(&bbox1); @@ -3691,4 +3801,75 @@ mod tests { ); } } + + /// Two overlapping bilinear patches in the same plane, the second one shifted. + fn overlapping_pair() -> (Surface, Surface) { + let patch = |x: f64| { + Surface::from( + BSplineSurface::create_bilinear_surface( + Vec3::new(x, 0.0, 0.0), + Vec3::new(x + 2.0, 0.0, 0.0), + Vec3::new(x, 3.0, 0.0), + Vec3::new(x + 2.0, 3.0, 0.0), + ) + .unwrap(), + ) + }; + (patch(0.0), patch(1.0)) + } + + #[test] + fn boundary_memo_hit_is_the_fresh_build() { + let on = Kernel { + memoize_curve_caches: true, + ..Kernel::default() + }; + let off = Kernel { + memoize_curve_caches: false, + ..Kernel::default() + }; + let (a, b) = overlapping_pair(); + let want = format!( + "{:?}", + a.base_coincidence_check(&off, &b, 1e-6, false).unwrap() + ); + assert!(a.boundary_memo().0.get().is_none()); + let (mut a, b) = overlapping_pair(); + let first = format!( + "{:?}", + a.base_coincidence_check(&on, &b, 1e-6, false).unwrap() + ); + assert!(a.boundary_memo().0.get().is_some()); + assert!(b.boundary_memo().0.get().is_some()); + let second = format!( + "{:?}", + a.base_coincidence_check(&on, &b, 1e-6, false).unwrap() + ); + assert_eq!(first, want); + assert_eq!(second, want); + // a copy starts empty, an edit empties the memo + assert!(a.clone().boundary_memo().0.get().is_none()); + let _ = a.kind_mut(); + assert!(a.boundary_memo().0.get().is_none()); + // other tolerances miss and build again, with the same result + let mut loose = Kernel { + memoize_curve_caches: true, + ..Kernel::default() + }; + loose.tolerances.zone_tol_3d *= 10.0; + let (a2, b2) = overlapping_pair(); + let _ = a2.base_coincidence_check(&on, &b2, 1e-6, false).unwrap(); + let fresh = format!( + "{:?}", + overlapping_pair() + .0 + .base_coincidence_check(&loose, &b2, 1e-6, false) + .unwrap() + ); + let missed = format!( + "{:?}", + a2.base_coincidence_check(&loose, &b2, 1e-6, false).unwrap() + ); + assert_eq!(missed, fresh); + } } diff --git a/crates/solidcore/tests/curve_cache_memo.rs b/crates/solidcore/tests/curve_cache_memo.rs index e40e8cab9d..696d7f603c 100644 --- a/crates/solidcore/tests/curve_cache_memo.rs +++ b/crates/solidcore/tests/curve_cache_memo.rs @@ -95,17 +95,30 @@ fn operands(k: &mut Kernel, name: &str) -> solidcore::Result<(Brep, Brep, Op)> { api::create_box(k, p(-30.0, -30.0, 10.0), 60.0, 60.0, 20.0)?, Op::Intersection, ), + // analytic surfaces against B-spline surfaces (both memoise their boundaries) + "skin-minus-cyl" => ( + skin(k, 20.0, 12.0, 0.0, 40.0, 5)?, + api::create_cylinder(k, p(0.0, 0.0, -5.0), 4.0, 50.0)?, + Op::Difference, + ), + "skin-and-sphere" => ( + skin(k, 20.0, 12.0, 0.0, 40.0, 5)?, + api::create_sphere(k, p(5.0, 0.0, 20.0), 12.0)?, + Op::Intersection, + ), _ => unreachable!("unknown case {name}"), }) } -const CASES: [&str; 6] = [ +const CASES: [&str; 8] = [ "box-cyl-hole", "sphere-and-box", "torus-minus-box", "cyl-union-cyl", "skin-minus-skin", "skin-and-box", + "skin-minus-cyl", + "skin-and-sphere", ]; /// Every observable output of one case as text (`{:?}` prints each `f64` so that it diff --git a/plan/findings/surface.surface_p8--coincidence-boundary-memo.md b/plan/findings/surface.surface_p8--coincidence-boundary-memo.md new file mode 100644 index 0000000000..fc2d1044ca --- /dev/null +++ b/plan/findings/surface.surface_p8--coincidence-boundary-memo.md @@ -0,0 +1,31 @@ +# surface.surface_p8: CoincidenceCheck keeps each surface's boundary curves (2026-10-25) + +Unit: `surface.surface_p8` (`SmSurface::CoincidenceCheck`, base body), with +`surface.surface_p2` (the `Surface` field). Not upstream: this goes beyond the C++. + +**Upstream.** `CoincidenceCheck` (`SmSurface.cpp:7553-7580`) calls +`CreateNaturalUVTrimCurves` on both surfaces for every check and deletes the curves at +the end (`SmObjsDelete`), so their `SmCacheMgr` entries go with them. A boolean runs the +check for every face pair, so the C++ builds the same boundary curves and their curve +caches again for every pair. On the #948 loft at 20 sections both kernels made 600 +curve-cache builds there (gdb on `Tessellate`), 30 % of the C++ samples at 40 sections. + +**Now.** Each `Surface` value keeps the result of its natural boundary curves in a memo +(`BoundaryMemo`, a `OnceLock`), keyed by the domain's bits and the kernel inputs of the +build (`CurveMemoKey::of_kernel`: tolerances and the context scalars). The check +borrows the curves (it only reads them), and the curves carry their own curve cache +memos (`plan/findings/curve.curve_p5--curve-cache-memo.md`), so each boundary curve's +cache is built once per surface value. Only B-spline, plane, cone, sphere and torus +surfaces are memoised: their iso-curve overrides read nothing but their own value. A +copy of a surface starts with an empty memo; an edit through `&mut Surface` replaces it. +One entry per live surface, freed with it. No `unsafe`; `Surface` stays `Send`. + +**Why no result can depend on it.** `CreateNaturalUVTrimCurves` takes `&Kernel` and the +surface; its result is a function of the surface value, the domain and the key, so a +hit gives what a fresh build gives. `Kernel::memoize_curve_caches` (and the +`curve-memo-off` feature) turns it off; the unit test +`boundary_memo_hit_is_the_fresh_build` and `tests/curve_cache_memo.rs` compare both +positions bit for bit. + +**After.** Same loft and method: 128 builds in `CoincidenceCheck` (C++ 600), 937 in the +whole run (C++ 1,393).