Repository navigation
Design question: where should edge-curve caches live, so that a boolean builds each curve cache once? #982
Description
Suggested labels: enhancement, question.
A question for the maintainers before we write more _cached plumbing. After #951 and #952, outer - inner of the #948 lofts at 40 sections takes 11.4 s in solidcore and 3.4 s in the C++ (measured, median of 3, release builds). Most of the remaining gap is curve caches that the port builds again on each call, where upstream builds them once and keeps them with the curve. We propose to keep them on the Curve value, as #697 already does for surfaces (option C below). Which ownership do you want?
What happens
Profile of the 40-section run with #951 and #952 applied (perf, frame pointers; shares of all samples, measured):
| Caller | Share | Curves |
|---|---|---|
curve_on_classify, its edge intersections |
16 % | B-rep edge curves |
ProtoTopology::split_intersecting_proto_edges |
7 % | proto-edge curves (imprint and merge) |
CoincidenceCheck boundary curves |
22 % | surface boundaries; same build count as upstream, out of scope here |
#951 also names FindBoundaryStartPoints and Loop::ContainsUVPoint. With the first two rows, these took 17 % (40 sections) to 28 % (20 sections) of all samples, measured with #951 only, before #952.
Cause
Upstream keeps one SmCurveCache per curve in the context's global cache (SmCacheMgr::GetOrCreateObjectCache, SmCacheMgr.cpp:737):
- The first solve on a curve builds the cache. Every later solve on that curve object uses it.
SmCurve::Notifydeletes it onSM_NO_PRE_EDITandSM_NO_DESTRUCTION(SmCurve.cpp:19354).- The queue holds at most
SM_DEFAULT_MAXCOUNT_CURVECACHE= 1000 curve caches (SmConfig.h:91). Eviction changes only speed, because a rebuild gives the same cache.
The port has no global state, so a cache lives only as long as the caller keeps it (global_point_solve_cached, global_curve_intersect_cached, CurveOnClassifyCache, find_boundary_start_points_with_caches). kernel::Caches (kernel.rs, "upstream SmCacheMgr*") is still empty. Each fix so far (#539, #753, #951, #952) keeps caches for one caller. The callers above solve on curves that the B-reps and the proto topology own, across many calls, so a per-caller fix does not reach them.
Requirement: the cache must be invisible
A cache that only memoizes a deterministic function of immutable geometry is not state in the sense of the "no global state" goal: with or without it, every output is bit-identical, and no result depends on what was solved before. Whatever option is chosen, we propose to make that a tested property:
- a switch (feature or test setting) that turns every kept curve cache off;
- a test that runs representative operations both ways and asserts bit-identical outputs, and difftest runs with the switch in both positions;
- no dependence on solve order, thread or clone.
Options
All three keep the goals: no unsafe, no global state, deterministic, wasm32. A kept curve cache is the cache a fresh build makes (make_curve_cache has one option set), so outputs stay bit-identical in each option, as in #951.
A. Map on the Brep |
B. Per-operation context | C. Slot on the Curve |
|
|---|---|---|---|
| Where it lives | Brep field, BTreeMap<EdgeId, CurveCache> |
a CurveCaches map that the boolean owns (as ArenaPair owns FaceCaches in #952), lent down |
Curve field, Arc<Mutex<..>> slot, as Surface's CacheSlot (#697) |
| Invalidation | drop an edge's entry at Brep::edge_mut and edge removal |
empty all maps at any mutable lend of a B-rep (the #952 rule) | clear at Curve::kind_mut and every &mut geometry edit (= SM_NO_PRE_EDIT); dropped with the curve |
| Against upstream | stricter: any edge_mut drops it (upstream: a curve edit only) |
stricter: any edit drops all | the same rule as upstream |
| API impact | new Brep field; the edge solve sites read it (&Brep needs interior mutability to fill it) |
a &mut CurveCaches parameter through curve_on_classify, split_intersecting_proto_edges, the boundary start points and ContainsUVPoint |
none at the about 90 solve sites; CurveCache must not store the caller's handle O, or must take it at solve time |
| Covers | B-rep edges only (16 % row) | the call chains it is threaded through | every solve on a curve object (both rows, other callers too) |
| Gain (estimate) | up to 1.8 s of 11.4 s | up to 2.6 s, less in imprint and merge, where edits empty the maps | up to 2.6 s |
| Main risk | 185 edge_mut calls in 48 files drop caches that are still valid |
the most signature changes; gains stop at each edit | memory; solve sites that pass Curve::clone get an empty slot |
Notes:
- Estimates. "Up to" assumes run time scales with the sample share, and that every build in that row goes away. Upstream also builds each cache once, so the true gain is smaller. If one option removes all of the 23 %, the 40-section difference is about 8.8 s, still 2.6x the C++. The
CoincidenceCheckwork is separate. - A.
split_intersecting_proto_edgessolves on proto-edge curves, which theBrepmap does not hold. A also adds a second edit path (edge_mut) that must stay in step with the curve. - B. This continues perf: keep face surface caches across the face pairs of a boolean (outputs bit-identical) #952 with the least new design. It does not help callers outside the boolean, such as offset, blend and healing.
- C.
Clonegives an empty slot (upstream's copy constructor leavesm_pCacheObjnull), andclone_for_evaluationshares the slot, as for surfaces. The cache holds its ownCurvecopy, whose slot is empty, so a slot never owns itself. Memory has no bound like upstream's 1000. It grows with the live curves that have had a solve, and aKernel::caches.clear()cannot reach it. The cache build readsk.tolerances, so a curve that is solved under two kernels needs the tolerances in the key, or a check. Kernel::caches. AGENTS.md namesKernelas the home for caches. A map there needs a curve identity (notify_identity) and an edit notification.Curve::kind_muthas no&Kernelto send one, so a kept cache can go stale. For this reason we did not list it as an option.
Question
- Is C acceptable, given the precedent of the
Surfaceslot? If not, do you prefer B, or an A/B mix (B for the boolean, later A)? - For C: is an unbounded per-curve slot acceptable, or do you want a size or count bound, and where?
- For C: may
CurveCachestop holding the caller's handleO(the tree owner), so that one cached tree serves every handle space? Or do you prefer one slot for each handle space, asTrimCacheSlotkeepsEntityRef?
Not measured
-
A draft PR with a prototype of option C and this switch follows, so the options can be compared on measured numbers. It is a proposal for this question, not a decision.
-
The gains in the table are estimates from profile shares. We did not build any of the options.
-
We did not count the cache builds in the C++ for these callers.
-
We did not audit how many solve sites pass a plain
Curve::clone(C) or reach the curve throughedge_mut(A).
Related
- SMLib boolean: difference of two capped ruled skins fails with SM_ERR (1001), no radial sector found (C++ and Rust alike) #948: the input (lofted rain-cover skins).
- perf: keep curve caches across repeated solves on the same curve (outputs bit-identical) #951: curve caches across repeated solves in
CoincidenceCheckandFindInteriorCurves. - perf: keep face surface caches across the face pairs of a boolean (outputs bit-identical) #952: face trimmed-surface caches across the face pairs of
IntersectTopology; the invalidation rule that option B reuses. - port: face-bound trimmed surface cache and IntersectTSBound's cached branch #697: the trimmed surface cache kept on the
Surface(the precedent for C). Model lazy surface cache trees: GetDomainWithContinuity depends on whether the cache tree was built #568: cache history changes results for surface trees. Curve caches build the full tree at once, so we expect no such effect. - perf: memoise surface-cache pole data, keep offset curve caches (outputs bit-identical; #506) #539, fix: keep curve caches on the curves across IntersectAndTrimOffsets pairs (composite_curve_p2) #753: earlier per-caller curve-cache fixes (
IntersectAndTrimOffsets). - perf: topology_traverser is_inner_shell degenerate-weight case 50x slower than C++ (keep the surface cache across rays) #496: the surface cache kept across the rays of
IsInnerShell.
Activity
meros commentedon Oct 25, 2026
Measured since filing: exact curve-cache build counts in both kernels, so the "Not measured" item on C++ build counts is now answered.
Method: 20-section case of #948, solidcore main
4fe6f4d51(with #951 and #952), pinned C++ reference. EverySmCurveCache::Tessellate(C++) and everyCurveCache::tessellate(Rust, all 32 monomorphs) was caught with a gdb breakpoint and a backtrace, and classified by caller. These are exact counts, not samples.diff=outer - inner,isect= hollow ∩ box.SplitIntersectingProtoEdgesIntersectEdges(curve_on_classify)ContainsUVPointFindBoundaryStartPointsFindInteriorCurvesCoincidenceCheckGlobalPointSolveDoStitching)Two corrections to the issue text:
CoincidenceCheckis not part of the gap. It builds the same number of caches (602 against 600), and one build costs about the same: 1.96 ms against 1.78 ms (perf, cycles). Its leaf profile matches routine for routine (basis_derivs_into375 againstsm_BasisDerivs343 samples), so the port builds no deeper tree and uses no other tolerance.curve_on_classifyandSplitIntersectingProtoEdges, and 5x to 38x inContainsUVPoint. That supports option C (or any option that reaches proto-edge curves), because upstream builds each of these once and keeps it on the curve.The per-build comparison is from 20 sections only; the 40-section counts were not taken.
meros commentedon Oct 25, 2026
Refinement of option C (the slot on the
Curve), worked out while starting the prototype. In short: make it a pure memo ofmake_curve_cache, keyed by every input of that function, so that it cannot change a result.Changes against the option C row above:
Curve::cloneshares the memo instead of starting empty, because the copy has the same geometry. An edit to either copy replaces only that copy's memo.&mutpath to the geometry (kind_mut,set_owner, the 5match &mut self.kindincurve/curve/p2.rs) replaces the memo with an empty one.The principle: a cache that nobody can observe
A cache is global state only if a result can depend on it.
make_curve_cache(upstreamSmCurve::CacheMakeOrValidate) is a deterministic function of:CurveKind: control points, knots, weights);Unportedtoday,curve_cache.rs,curve_approx_tol_3d);&Kernel: the tolerances and the context's approximate tolerance.It takes
&Kernel, so it cannot draw from the RNG or writeupstream_ub_passed, andContexthas no interior mutability. #951 already relies on this: a kept cache is the cache that a fresh build makes. A memo of this function, keyed by all of its inputs, cannot change a result. It can change only the time.Conditions for an invisible memo, and how to test them
Kernelfield, read through&Kernel). A test runs representative operations with the memo on and off and compares every output bit by bit. The difftest of the touched units must keepmax_rel_err=0e0. The SMLib boolean: difference of two capped ruled skins fails with SM_ERR (1001), no radial sector found (C++ and Rust alike) #948 loft outputs must be identical to main.Curve::cloneshares the memo: the copy has the same geometry, so the entry is valid for it. An edit to either copy replaces only that copy's memo. The cache keeps its own copy of the curve without a memo, so a memo never holds itself.Arc<Mutex<..>>isSend + Syncand needs nounsafe. It compiles forwasm32-unknown-unknown, as theSurfacecache slot of port: face-bound trimmed surface cache and IntersectTSBound's cached branch #697 does.CurvestaysSendandSync. Two threads that fill one entry build the same value, so the race decides only who builds it.Where it does not fit
CurveCache<O>stores the caller's handle as the tree owner. A shared entry must be handle-neutral, and a hit must give a copy with the caller's handle. The copy costs a clone of the spans, which is less than a build. We did not measure how much less.CoincidenceChecksolves on boundary curves made for each check, so a memo on the curve value does not reach them. That work stays separate.*_cachedAPIs. They still save the clone that a hit costs.Updated questions (replacing 2 and 3 above):
KernelOptions) or test-only?The prototype PR follows and will report build counts per caller against the C++ counts in the comment above.
meros commentedon Oct 25, 2026
Cache requests and hits, next to the build counts above. This tests whether the port solves more often than upstream, or only rebuilds more often.
Method: 20-section case of #948, main
d4940b63e, exact gdb counts. C++: everySmCacheMgr::GetOrCreateObjectCachewithSM_OC_CURVE(a request) and everySmCurveCache::Tessellate(a build). Rust: the request sites (surface/surface/p8.rs:409,:412,curve/curve/p1.rs:1673) and everyCurveCachetessellate.SplitIntersectingProtoEdgesCoincidenceCheckFindInteriorCurvesIntersectEdges(curve_on_classify)ContainsUVPointIntersectVerticesSplitIntersectingProtoEdgesContainsUVPoint(
≥: some Rust sites build without passing the instrumented request lines, so the builds are the lower bound.)meros commentedon Oct 25, 2026
Prototype of option C (as refined above): #992 (draft, waiting for the decision here).
Measured on the #948 lofts, whole run, instruction counts (vary by less than 0.2 %):
All outputs are identical, and a new test runs 6 booleans with the memo off and on and compares every value bit for bit. Details and the remaining hotspots are in #992.
Two corrections to the refinement comment above, found while building it:
CurveisSendbut notSyncon main (CrvOnSurfand the offset surface holdCellandRefCell). The memo keeps itSendand notSync; the claim "CurvestaysSendandSync" was wrong.meros commentedon Oct 25, 2026
Answered by merging #992 (option C: the curve-cache memo in the curve value, with
Kernel::memoize_curve_cachesand thecurve-memo-offfeature). Follow-ups: #1031 (boundary curves kept per surface, stacked on #992) and #1032 (handle-agnostic caches).