chore: Migrate cross-product lookups to use bitmask isntead of CommutativeDuplex - BED-8362#2821
chore: Migrate cross-product lookups to use bitmask isntead of CommutativeDuplex - BED-8362#2821StephenHinck wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesEager Check Set Materialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/go/analysis/ad/ad.go (1)
618-624: 💤 Low valueConsider reusing
setUnionbitmap across iterations.A new bitmap is allocated each iteration. Since
setUnionis discarded after eachAndoperation, you could hoist the allocation and callClear()at the start of each iteration to reduce GC pressure in pathological cases with many nodesets.♻️ Suggested optimization
+ setUnion := cardinality.NewBitmap64() for _, unrolledSet := range unrolledSets[2:] { - setUnion := cardinality.NewBitmap64() + setUnion.Clear() for _, bm := range unrolledSet { setUnion.Or(bm) } materializedCheckSet.And(setUnion) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/go/analysis/ad/ad.go` around lines 618 - 624, The loop repeatedly allocates a new bitmap via cardinality.NewBitmap64() for setUnion; instead, allocate setUnion once before iterating over unrolledSets[2:], and inside the loop call setUnion.Clear() before OR-ing each bm, then use materializedCheckSet.And(setUnion) as before—this reuses the same bitmap instance (referencing variables unrolledSets, setUnion, materializedCheckSet and the constructor cardinality.NewBitmap64) to reduce GC pressure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/go/analysis/ad/ad.go`:
- Around line 618-624: The loop repeatedly allocates a new bitmap via
cardinality.NewBitmap64() for setUnion; instead, allocate setUnion once before
iterating over unrolledSets[2:], and inside the loop call setUnion.Clear()
before OR-ing each bm, then use materializedCheckSet.And(setUnion) as
before—this reuses the same bitmap instance (referencing variables unrolledSets,
setUnion, materializedCheckSet and the constructor cardinality.NewBitmap64) to
reduce GC pressure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7be73307-5588-46a8-9d82-4ae436286ac0
📒 Files selected for processing (1)
packages/go/analysis/ad/ad.go
| } | ||
|
|
||
| // This means that len(firstDegreeSets) must be greater than or equal to 2 i.e. we have at least two nodesets (unrolled) without Auth. Users/Everyone | ||
| checkSet.Or(cardinality.CommutativeOr(unrolledSets[1]...)) |
There was a problem hiding this comment.
I would like to understand the motivation behind #2364 before moving forward with this changeset. The commutative work was done fairly recently seemingly in response to observed bottlenecks that this changeset will undo.
Perhaps we need to have the code in place for both and pivot based on the graph shape instead of always doing or the other.
There was a problem hiding this comment.
After reviewing more closely I see that the contains check is where we currently have an excess of computation since we have to check every bitmap in the and slice for a commutative duplex. That combined with contains checks being done three times inside of different loops is way more processing than up front intersection that then benefits from direct contains checking with no additional looping. Nice
StephenHinck
changed the title
| checkSet.Or(cardinality.CommutativeOr(unrolledSets[1]...)) | ||
| // Materialize the check set as a single bitmap by Or-ing all component bitmaps within each set, then And-ing across sets. | ||
| materializedCheckSet := cardinality.NewBitmap64() | ||
| for _, bm := range unrolledSets[1] { |
There was a problem hiding this comment.
nit: The changeset is good but would appreciate updating bm to bitmap or set in this loop and the nested loop below for a bit more clarity
|
|
||
| // This means that len(firstDegreeSets) must be greater than or equal to 2 i.e. we have at least two nodesets (unrolled) without Auth. Users/Everyone | ||
| checkSet.Or(cardinality.CommutativeOr(unrolledSets[1]...)) | ||
| // Materialize the check set as a single bitmap by Or-ing all component bitmaps within each set, then And-ing across sets. |
There was a problem hiding this comment.
nit: While Or and And should be understood given the context, the bitmap methods are named this way already so I suggest updating the comment here to spell out union and intersection so that it adds more insight than seeing the code itself.
| // Materialize the check set as a single bitmap by Or-ing all component bitmaps within each set, then And-ing across sets. | |
| // Materialize the check set as a single bitmap from the union of all component bitmaps (`Or`) within each set, then intersecting (`And`) across sets. |
2 participants
Description
Migrates
CalculateCrossProductNodeSetsto utilize a bitmap instead ofCommutativeDuplexfor performance optimization. In pathological cases, this resulted in a 98% reduction in post-processing time for ADCS.Motivation and Context
Resolves BED-8362
Why is this change required? What problem does it solve?
How Has This Been Tested?
Existing test suites are unchanged. Additionally, performed manual validation utilizing several local datasets.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit