_get_or_add_point!: O(1) stale-tag probe + closest-of-many picker - #280
Open
ybrightye wants to merge 2 commits into
Open
_get_or_add_point!: O(1) stale-tag probe + closest-of-many picker#280ybrightye wants to merge 2 commits into
_get_or_add_point!: O(1) stale-tag probe + closest-of-many picker#280ybrightye wants to merge 2 commits into
Conversation
added 2 commits
August 10, 2026 20:35
Two-in-one follow-up from #260: 1. Replace `only(pts).val` with a closest-by-squared-distance picker so multi-candidate collisions (which happen when callers query with a wider `atol` than the tightest inserted spacing) resolve deterministically instead of erroring. 2. Replace the O(N) `k.get_entities(0)` scan (previously run on every cache-hit) with an O(1) `k.getBoundingBox(0, tag)` probe. If the probe throws (stale OCC tag left over from a `k.cut` retag/delete), the stale entry is dropped from the RTree and the query retries. Measured: at N=10k points, cache-hit time drops from ~105 µs to ~12 µs (~8.7x faster) with no change to insert-phase time. This is the inner-loop bottleneck for chip-scale renders that reuse a shared points_tree across many `_add_loop!` calls. Correctness: added two testsets exercising both fixes, including a regression test that removes an OCC point out from under the RTree and verifies the next lookup gets a fresh tag. Addresses gpeairs' comment on #260 (2026-08-07): "avoiding the O(N^2) sweep over points and relaxing the `only` call to allow for multiple merge candidates."
CI on all 4 Julia versions surfaced two bugs in the stale-tag testset: 1. `length(::RTreeRegionQueryIterator)` isn't defined — replaced with `collect(...)` before checking length. 2. `@test fresh != stale` failed because OCC recycles freed tag values (verified: after `k.remove([(0, 1)])`, the next `k.add_point` gets tag 1 again). The real invariant is that the returned tag is a live OCC entity, not that its value differs. Rewrote to assert liveness via `getBoundingBox`, and asserted that `stale` throws immediately after `k.remove` (before any recycling).
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Follows up on @gpeairs's comment in #260 (2026-08-07): "another patch of yours that didn't make it here touched
_get_or_add_point!, avoiding the O(N^2) sweep over points and relaxing theonlycall to allow for multiple merge candidates. Does it make sense to upstream that?" — this ships both.Changes
Closest-of-many picker on multi-candidate collisions. Replace
tag = only(pts).valwith a squared-distance scan acrosspts. Callers that query with a wideratolthan the tightest inserted spacing (e.g. rendering pipelines that use a relaxed 2 nm vertex-merge tolerance while inserting arc centers at strict tolerance) can catch >1 candidate; the old code errored, the new one returns the closest.O(1) stale-tag probe instead of the O(N)
k.get_entities(0)scan. The check that catches a stale RTree tag afterk.cutretagged/deleted the underlying OCC point is nowk.getBoundingBox(0, tag)in atryblock — throws for a stale tag, returns for a live one. On stale, the entry is dropped from the RTree so the next query doesn't hit it again.Benchmark
Micro-benchmark at N=10,000 points (hot cache-hit path — the loop
_add_loop!runs during a full-chip render):At production scale (a chip render inserts ~40 k points and re-queries each many times through shared boundaries) this is the difference between the point-lookup path being a bottleneck and being negligible.
Correctness
Two new testsets under
test/test_solidmodel.jl:_get_or_add_point! multi-candidate resolution— inserts two points 0.1 µm apart, queries with a wideatol=1.0that catches both, asserts the closer one is returned in both directions._get_or_add_point! recovers from stale RTree tag— inserts a point, callsk.remove([(0, tag)])to simulate ak.cutthat took it out from under the tree, asserts the next query returns a fresh tag and the stale entry is dropped.Pkg.testlocally: 595/595 pass (was 553; +7 new assertions across the two testsets).Formatter clean (
scripts/format.jl check).