You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
perf(boolean): a list operand's merges keep one record store (#3261) - #3264
boolean_lists merges its N tools into one operand through one record store instead of a new one for each merge: 930 N + 866 visits in place of about 300 N² (24 116 in place of 199 344 at 25 tools, 93 866 in place of 3 049 794 at 100). No result changes.
The problem
boolean_lists makes one B-rep of each list before it runs the boolean (boolean::merge::p3::list_operand): it copies the first B-rep and merges each other one into the copy (MergeBrep). A merge notifies through a record store that holds a default SmTopology record for every object of the operand and for the operand itself. Each merge started from an empty store, so each merge checked and seeded every arena slot of the operand so far: the N merges of a batch cut visited the square of N slots.
The change
list_operand keeps one store for its merges (combine_brep_keeping). Before each merge the kept store is left exactly as a new one would be when the merge starts: a default record for every object of the operand and for the operand, and no other (reseed_records, the function the stitch's glues and the sweep's faces already use for their kept store). It runs under the names the merge itself uses (notify_ref for the objects, brep_ref for the operand).
The second merge resets the whole kept store once (it was filled by the first merge, not by a reset, so nothing says what changed). From the third merge on a reset visits what the last merge changed, added and removed, as long as the store's change tracking holds; a merge that removes a record or changes records in bulk makes the next reset a full one again.
combine_brep is unchanged for its other caller (the offset's single merge): it starts from an empty store.
WorkCounters::merge_record_visits counts the store's work, for the test: one for each arena slot, key or record a pass touches (a slot scanned for a live object, emptied ones included; a key made, compared, matched, removed or added; a record reset), in the key check and the seeding of the operand, the key check and the seeding of the B-rep merged in, and a reset. Outside the count: the ten kinds' own bookkeeping, the progress entries (one for each B-rep value) and the steps of the map's tree. The tally is kept in NotifyRecords where each pass runs; no result reads it.
The store is local to one list_operand call: no kernel state, nothing shared between calls or threads.
Counts
Plate 20 n x 20 n x 12 minus N = n² cylinders r 4 in one boolean_lists call, SolidCraft's fixes:
N
Visits before
Visits after
25
199 344
24 116
100
3 049 794
93 866
200
12 100 394
186 866
Law
about 300 N²
930 N + 866
25 and 100 are the committed test's own output before and after the fix; 200 was measured by a reviewer.
Why no result changes
The store a merge's edit runs on holds the same keys with the same default records whether it was made new or kept and reset, so the merge, its notifications and the attributes it propagates are the same.
A unit test merges boxes and cylinders with an attribute on every face both ways (a kept store, a new store for each merge), fixes on and off, and compares the operand's whole Debug dump and the records left after every merge.
Two independent reviews were told to break exactness and accepted the change: 34 and 38 old-against-new public results byte for byte equal (bodies, attributes, errors, failures and cancellation included), and adversarial probes of the merges, partial failures and the reset's invariants among them. Their first two rounds rejected the counter alone, for passes it left out; all are counted now.
One thing differs in the B-rep value and is not a result: the operand's removal log gets its tag (Brep::removal_tag), which reseed_records names the value by. Every stitched B-rep has it; it is not serialized, and a copy or a rollback starts a new one. Removals are logged whether or not the tag exists.
Tests
list_operand_merges_visit_records_in_proportion_to_the_tools (committed first, failing with (199344, 3049794) against (24116, 93866)): the counts, with the cut's faces, edges and exact volume.
merges_that_keep_their_record_store_make_the_same_operand: as above.
Verification
On 0dde1c534, rebased on main dc5cfd7e7, Linux (WSL Ubuntu 22.04):
Goldens, --all --goldens: goldens: 283 unit(s), 282 passed, 1 failed. The one is curve.conic case gen2235 (values.r[4]: 3.776586443654626e-14 -> 3.552713678800501e-14), which differs the same way on unchanged main on that machine (checked on ba55b2f71 and on 7b29ffdb5); this change does not touch it.
The count test at the test commit fails with (199344, 3049794); at the head it passes with (24116, 93866). boolean_api, boolean_no_dangling_vertex_refs, issue_1120_boolean_slits pass. The focused lib tests (boolean::merge, topology::brep::p3, topology::topology, kernel::tests) pass: 131.
On the earlier base ba55b2f71, together with four other changes on one branch: the whole workspace suite (7181 passed, 113 skipped) and the kernel rig as CI runs it (1081 cases, 803 pass, 0 fail, 15 known-failing, none moved). Not re-run on this head: CI is the gate.
Not attached
A schematic. The mechanism is bookkeeping (a map rebuilt for each merge); a picture of the body does not show it, and I have no VectorCraft figure of it. Say if you want one before merging.
Not in this PR
The merge itself still copies each tool's objects one by one (linear).
The three stitches of the list path that glue no edge.
copy_component and the other callers that make a store per edit.
No actionable comments were generated in the recent review.
Walkthrough
List-operand merges now reuse notification records across successive B-rep merges. The change tallies record-processing visits in kernel counters and adds tests for merge equivalence, result geometry, and visit counts.
NotifyRecords and B-rep record operations tally visits. WorkCounters exposes, increments, and resets the merge-record visit count.
Reuse records across list merges crates/solidcore/src/boolean/merge/p3.rs
list_operand passes a retained notification-record store through successive merges. A test compares the resulting B-reps and records with merges that use a fresh store.
Validate merge visits at scale crates/solidcore/tests/list_operand_merge_work.rs
An integration test checks result geometry and asserts visit counts of 24,116 for 25 tools and 93,866 for 100 tools.
Change: Refactor · Severity of issue fixed: Medium
Sequence Diagram(s)
sequenceDiagram
participant list_operand
participant combine_brep_keeping
participant NotifyRecords
participant WorkCounters
list_operand->>combine_brep_keeping: pass retained record store
combine_brep_keeping->>NotifyRecords: reseed and update records
combine_brep_keeping->>WorkCounters: add record visit tally
combine_brep_keeping-->>list_operand: return updated record store
This change reuses internal merge records within a single list-operand boolean call and adds a work counter. Geometry results are reported as unchanged and are covered by equivalence and scale tests. No actionable merge-blocking risk remains.
Security architecture risk: Minimal · up to 0dde1
The reused records remain confined to one list-operand construction. Source inspection supports preservation of identity checks, fresh-store defaults and error handling. The new work counter grants no additional access, and no material security risk was identified.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The changed state-sharing scope is successive geometry merges within one list operand, not a cross-request or cross-tenant cache. The examined change affects in-process geometry state and diagnostic counts without introducing a new privileged destination.
Trust Boundaries and Controls
observed — The merge still passes through reserved-key validation for the target and source before editing. Cached validation progress is bound to both the identity scheme and B-rep instance; ordinary cloning gives a distinct progress identity. Reseeding does not remove these checks.
Resilience and Maintainability Implications
observed — On Result-based failures, attribute restoration and return of the record store occur before the merge error propagates. The existing policy still propagates cancellation and optionally propagates ordinary merge errors. If an ordinary error is ignored, the next iteration reseeds the retained records; exiting the list drops its local store. This is cleanup, not transactional rollback of a partially edited B-rep.
Pre-merge checks: 4
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title names the main change: a list operand's merges now share one record store, which is the performance change described in the raw summary and objectives. It also uses the conventional perf(boo…
Linked Issues check
Directly linked issue #3261 has one coding requirement group. The store used by the merges of a boolean_lists list operand must hold the same keys and default records as a new store, and record-stor…
Out of Scope Changes check
The changes trace to #3261. - list_operand and combine_brep_keeping in boolean/merge/p3.rs implement the fix. The combine_brep wrapper keeps the other caller's behavior. - The visit tally in `…
Full details: Title check
Explanation
The title names the main change: a list operand's merges now share one record store, which is the performance change described in the raw summary and objectives. It also uses the conventional perf(boolean) prefix. The title is specific and does not describe unrelated work. The PR number in the title (#3261) differs from the PR number in the objectives (#3264), but this does not affect whether the title matches the change.
Full details: Linked Issues check
Explanation
Directly linked issue #3261 has one coding requirement group. The store used by the merges of a boolean_lists list operand must hold the same keys and default records as a new store, and record-store visits must grow with the tools instead of about 300 N².
Evidence from the reviewed diff of crates/solidcore/src/boolean/merge/p3.rs (merge base to head):
list_operand now creates one NotifyRecords and calls combine_brep_keeping for each source B-rep.
combine_brep_keeping calls reseed_records before each merge when the store is not empty. The scheme uses notify_ref, entity_of_notify_key and brep_ref(target), which are the names the merge uses. It then moves the store into ArenaNotifyState, runs the merge, and stores the records back.
combine_brep still starts from an empty store, so the offset caller keeps its behavior.
The test merges_that_keep_their_record_store_make_the_same_operand compares the operand Debug dump and the records after each merge, kept against fresh, with fixes on and off.
The author reports the count test values 24,116 and 93,866 (against 199,344 and 3,049,794 before) and the growth law 930 N + 866. I did not run the tests. The visit counts and the test results are author-reported evidence. The mechanism that yields linear growth is present in the code. Result equality rests on the in-tree test and on the reported reviews.
Full details: Out of Scope Changes check
Explanation
The changes trace to #3261.
list_operand and combine_brep_keeping in boolean/merge/p3.rs implement the fix. The combine_brep wrapper keeps the other caller's behavior.
The visit tally in NotifyRecords and WorkCounters::merge_record_visits is the measure #3261 names (WorkCounters::merge_record_visits). The public getter is test-facing and supports that measure.
Brep::removal_tag is needed by reseed_records, which names the removal log by that tag. The PR description states that the tag is not serialized and does not change results.
The integration test list_operand_merge_work.rs checks the scaling target from #3261.
No unrelated work appears in the description or the summaries. The PR excludes the per-object copy, the list-path stitches and copy_component, which fits the scope of #3261. I did not inspect the removal_tag and counter hunks in detail, so that part rests on the summary and description.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Reviewing files that changed from the base of the PR and between 165931d and 0dde1c5.
Files selected for processing: 4
crates/solidcore/src/boolean/merge/p3.rs
crates/solidcore/src/kernel.rs
crates/solidcore/src/topology/brep/p3.rs
crates/solidcore/src/topology/topology.rs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
…44, 3049794)
`boolean_lists` makes one operand of its N tools by merging them one at a time
(`list_operand`, `MergeBrep`). Each merge notifies through a record store with a
default record for every object of the operand so far; made from nothing for
each merge, the store costs the operand's size every time, so N merges visit the
square of N slots.
`WorkCounters::merge_record_visits` counts the work on the store of those
merges: one for each arena slot, key or record a pass of the key check, the
seeding or a reset touches (a slot scanned, emptied ones included; a key made,
compared, matched, removed or added; a record reset), tallied in
`NotifyRecords` where each pass runs. Outside the count: the ten kinds' own
bookkeeping, the progress entries and the map's tree steps. No result reads the
tally. The new test cuts 25 and 100
cylinders out of a plate in one call, checks the cut's faces, edges and exact
volume, and asks for 930 N + 866 visits: 24116 and 93866. It fails with 199344
and 3049794.
Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
`list_operand` merged each B-rep into the operand through a record store made
from nothing, so every merge gave a record to every object of the operand so
far: 199344 slot and key visits for 25 tools, 3049794 for 100 (the square of
the tools).
The merges now share one store. Before each merge it is left exactly as a new
one would be when the merge starts (`reseed_records`, as the stitch's glues and
the sweep's faces do with theirs: only what the last merge changed, added and
removed is visited), under the names the merge itself uses for the operand and
its objects. Visits: 24116 and 93866, 930 for each tool and 866.
The operand is the same: a unit test merges boxes and cylinders with attributes
both ways, fixes on and off, and compares the operand's whole `Debug` dump and
the records left after every merge. The one thing a kept store adds is the tag
of the operand's removal log, which any stitched B-rep has.
`combine_brep` (the offset's single merge) is unchanged: it starts from an
empty store.
Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3261
boolean_listsmerges its N tools into one operand through one record store instead of a new one for each merge: 930 N + 866 visits in place of about 300 N² (24 116 in place of 199 344 at 25 tools, 93 866 in place of 3 049 794 at 100). No result changes.The problem
boolean_listsmakes one B-rep of each list before it runs the boolean (boolean::merge::p3::list_operand): it copies the first B-rep and merges each other one into the copy (MergeBrep). A merge notifies through a record store that holds a defaultSmTopologyrecord for every object of the operand and for the operand itself. Each merge started from an empty store, so each merge checked and seeded every arena slot of the operand so far: the N merges of a batch cut visited the square of N slots.The change
list_operandkeeps one store for its merges (combine_brep_keeping). Before each merge the kept store is left exactly as a new one would be when the merge starts: a default record for every object of the operand and for the operand, and no other (reseed_records, the function the stitch's glues and the sweep's faces already use for their kept store). It runs under the names the merge itself uses (notify_reffor the objects,brep_reffor the operand).combine_brepis unchanged for its other caller (the offset's single merge): it starts from an empty store.WorkCounters::merge_record_visitscounts the store's work, for the test: one for each arena slot, key or record a pass touches (a slot scanned for a live object, emptied ones included; a key made, compared, matched, removed or added; a record reset), in the key check and the seeding of the operand, the key check and the seeding of the B-rep merged in, and a reset. Outside the count: the ten kinds' own bookkeeping, the progress entries (one for each B-rep value) and the steps of the map's tree. The tally is kept inNotifyRecordswhere each pass runs; no result reads it.The store is local to one
list_operandcall: no kernel state, nothing shared between calls or threads.Counts
Plate
20 n x 20 n x 12minus N = n² cylinders r 4 in oneboolean_listscall, SolidCraft's fixes:25 and 100 are the committed test's own output before and after the fix; 200 was measured by a reviewer.
Why no result changes
The store a merge's edit runs on holds the same keys with the same default records whether it was made new or kept and reset, so the merge, its notifications and the attributes it propagates are the same.
Debugdump and the records left after every merge.One thing differs in the B-rep value and is not a result: the operand's removal log gets its tag (
Brep::removal_tag), whichreseed_recordsnames the value by. Every stitched B-rep has it; it is not serialized, and a copy or a rollback starts a new one. Removals are logged whether or not the tag exists.Tests
list_operand_merges_visit_records_in_proportion_to_the_tools(committed first, failing with(199344, 3049794)against(24116, 93866)): the counts, with the cut's faces, edges and exact volume.merges_that_keep_their_record_store_make_the_same_operand: as above.Verification
On
0dde1c534, rebased on maindc5cfd7e7, Linux (WSL Ubuntu 22.04):cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,check_panic_sites.py,check_record_ids.py,check-frozen-archives.sh,cargo check -p solidcore --target wasm32-unknown-unknown: all exit 0.--all --goldens:goldens: 283 unit(s), 282 passed, 1 failed. The one iscurve.coniccasegen2235(values.r[4]: 3.776586443654626e-14 -> 3.552713678800501e-14), which differs the same way on unchanged main on that machine (checked onba55b2f71and on7b29ffdb5); this change does not touch it.(199344, 3049794); at the head it passes with(24116, 93866).boolean_api,boolean_no_dangling_vertex_refs,issue_1120_boolean_slitspass. The focused lib tests (boolean::merge,topology::brep::p3,topology::topology,kernel::tests) pass: 131.ba55b2f71, together with four other changes on one branch: the whole workspace suite (7181 passed, 113 skipped) and the kernel rig as CI runs it (1081 cases, 803 pass, 0 fail, 15 known-failing, none moved). Not re-run on this head: CI is the gate.Not attached
Not in this PR
copy_componentand the other callers that make a store per edit.🤖 Generated with Claude Code
Summary by CodeRabbit