Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBumps Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zinic
force-pushed
the
BED-7379
branch
2 times, most recently
from
bacc8ba to
a4bb4d9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/go/analysis/ad/ad.go (2)
542-558: Append toentityReachBitmapsprecedes exclusion guard — reorder for clarityLine 543 appends
entityReachSetstoentityReachBitmapsunconditionally, even though lines 545–558 may immediately setnodeExcluded = true, causing the entire slice to be discarded at line 567. The code is functionally correct, but reordering — check exclusion first, append only when safe — eliminates the unnecessary allocation on the excluded path and makes the intent clearer.♻️ Proposed reorder
entityReachSets := localGroupData.GroupMembershipCache.ReachSliceOfComponentContainingMember(entityID, graph.DirectionInbound) -entityReachBitmaps = append(entityReachBitmaps, entityReachSets...) for _, entityReachSet := range entityReachSets { if entityReachSet.Cardinality() > 0 { for _, excludedGroup := range localGroupData.ExcludedShortcutGroupsSlice { if entityReachSet.Contains(excludedGroup) { nodeExcluded = true break } } } if nodeExcluded { break } } + +if !nodeExcluded { + entityReachBitmaps = append(entityReachBitmaps, entityReachSets...) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/analysis/ad/ad.go` around lines 542 - 558, The code currently appends entityReachSets to entityReachBitmaps before checking exclusion, causing unnecessary allocations when nodeExcluded becomes true; change the order in the loop that calls localGroupData.GroupMembershipCache.ReachSliceOfComponentContainingMember so that you first iterate entityReachSets and test each entityReachSet against localGroupData.ExcludedShortcutGroupsSlice (setting nodeExcluded if any match) and only append entityReachSets to entityReachBitmaps when nodeExcluded is false; update references to entityReachSets, entityReachBitmaps, nodeExcluded, and ExcludedShortcutGroupsSlice accordingly to ensure the early-exit exclusion logic happens before appending.
509-509:unrolledSets[0]is allocated and populated but never readEvery non-excluded nodeset appends its
entityReachBitmapstounrolledSets(line 568), producingunrolledSets[0],unrolledSets[1],unrolledSets[2:]. However, thecheckSetpopulation at lines 588–592 starts at index[1]and[2:]—unrolledSets[0]is never accessed. The first nodeset's reach is handled exclusively viafirstDegreeSets[0]andGroupMembershipCache.OrReach(line 599), making the first slot's bitmap slice an always-wasted allocation. This directly undercuts the PR's memory-reduction goals.Consider skipping the append to
unrolledSetsfor the first non-excluded nodeset:♻️ Proposed refactor
- if !nodeExcluded { - unrolledSets = append(unrolledSets, entityReachBitmaps) - firstDegreeSets = append(firstDegreeSets, firstDegreeSet) - } + if !nodeExcluded { + // Only accumulate reach bitmaps for sets beyond the first; + // the first set is handled via firstDegreeSets[0] + OrReach. + if len(firstDegreeSets) > 0 { + unrolledSets = append(unrolledSets, entityReachBitmaps) + } + firstDegreeSets = append(firstDegreeSets, firstDegreeSet) + }Update the downstream index references accordingly (
unrolledSets[0]→ first non-first set,unrolledSets[1:]→ remainder).Also applies to: 568-568, 588-592
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/analysis/ad/ad.go` at line 509, The allocation for unrolledSets currently includes the first non-excluded nodeset but that slot is never read; change the logic that appends entityReachBitmaps so it skips appending for the very first non-excluded nodeset (i.e., only append subsequent nodesets into unrolledSets) and then update all downstream uses to account for the missing initial slot—replace uses that assumed unrolledSets[0] with the first nodeset's data from firstDegreeSets[0] / GroupMembershipCache.OrReach and shift index math where checkSet population and any loops use unrolledSets (e.g., the code building checkSet and the loop that reads unrolledSets[1:] should instead operate on the appended slice representing subsequent nodesets). Ensure references to unrolledSets, entityReachBitmaps, firstDegreeSets, and GroupMembershipCache.OrReach are updated consistently so no unused first slot is allocated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/go/analysis/ad/ad.go`:
- Around line 542-558: The code currently appends entityReachSets to
entityReachBitmaps before checking exclusion, causing unnecessary allocations
when nodeExcluded becomes true; change the order in the loop that calls
localGroupData.GroupMembershipCache.ReachSliceOfComponentContainingMember so
that you first iterate entityReachSets and test each entityReachSet against
localGroupData.ExcludedShortcutGroupsSlice (setting nodeExcluded if any match)
and only append entityReachSets to entityReachBitmaps when nodeExcluded is
false; update references to entityReachSets, entityReachBitmaps, nodeExcluded,
and ExcludedShortcutGroupsSlice accordingly to ensure the early-exit exclusion
logic happens before appending.
- Line 509: The allocation for unrolledSets currently includes the first
non-excluded nodeset but that slot is never read; change the logic that appends
entityReachBitmaps so it skips appending for the very first non-excluded nodeset
(i.e., only append subsequent nodesets into unrolledSets) and then update all
downstream uses to account for the missing initial slot—replace uses that
assumed unrolledSets[0] with the first nodeset's data from firstDegreeSets[0] /
GroupMembershipCache.OrReach and shift index math where checkSet population and
any loops use unrolledSets (e.g., the code building checkSet and the loop that
reads unrolledSets[1:] should instead operate on the appended slice representing
subsequent nodesets). Ensure references to unrolledSets, entityReachBitmaps,
firstDegreeSets, and GroupMembershipCache.OrReach are updated consistently so no
unused first slot is allocated.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/go/analysis/ad/ad.go (1)
568-569:unrolledSets[0]is built but never consumed — the per-entity appends for the first nodeSlice are wasted work.After this loop completes,
unrolledSetsis only accessed at indices[1](line 588) and[2:](lines 590–592). The element at index 0, along with allentityReachBitmaps = append(entityReachBitmaps, entityReachSets...)accumulations made for entities innodeSlices[0], are never read. TheReachSliceOfComponentContainingMembercalls fornodeSlices[0]are still necessary for the exclusion check, but the subsequentappendtoentityReachBitmapsand the resultingunrolledSets[0]entry are pure overhead.♻️ Proposed refactor: skip accumulation into `entityReachBitmaps` for `nodeSlices[0]`
- for _, nodeSlice := range nodeSlices { + for nodeSliceIdx, nodeSlice := range nodeSlices { var ( nodeExcluded = false firstDegreeSet = cardinality.NewBitmap64() entityReachBitmap = cardinality.NewBitmap64() - entityReachBitmaps = []cardinality.Duplex[uint64]{entityReachBitmap} + entityReachBitmaps = []cardinality.Duplex[uint64]{entityReachBitmap} // only used for nodeSliceIdx > 0 ) for _, entity := range nodeSlice { entityID := entity.ID.Uint64() firstDegreeSet.Add(entityID) entityReachBitmap.Add(entityID) if entity.Kinds.ContainsOneOf(ad.Group, ad.LocalGroup) { if localGroupData.ExcludedShortcutGroups.Contains(entityID) { nodeExcluded = true } else { entityReachSets := localGroupData.GroupMembershipCache.ReachSliceOfComponentContainingMember(entityID, graph.DirectionInbound) - entityReachBitmaps = append(entityReachBitmaps, entityReachSets...) + if nodeSliceIdx > 0 { + entityReachBitmaps = append(entityReachBitmaps, entityReachSets...) + } // exclusion check still runs for all nodeSlices for _, entityReachSet := range entityReachSets { ... } } } ... } if !nodeExcluded { - unrolledSets = append(unrolledSets, entityReachBitmaps) + if nodeSliceIdx > 0 { + unrolledSets = append(unrolledSets, entityReachBitmaps) + } firstDegreeSets = append(firstDegreeSets, firstDegreeSet) } }Note: if
unrolledSetsandfirstDegreeSetsare decoupled, the early-exitlen(firstDegreeSets)checks (lines 574–585) and theunrolledSets[1]/unrolledSets[2:]index math must be adjusted accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/analysis/ad/ad.go` around lines 568 - 569, The loop currently builds and appends per-entity accumulations into entityReachBitmaps for every nodeSlice, but the element at index 0 of unrolledSets is never read; update the loop so that when iterating nodeSlices[0] you still call ReachSliceOfComponentContainingMember for the exclusion check but skip appending to entityReachBitmaps and do not append an entry to unrolledSets for index 0; keep building firstDegreeSets as before (or else adjust the later len(firstDegreeSets) checks) and ensure subsequent code still reads unrolledSets[1] and unrolledSets[2:] as before. Specifically modify the code around the append(unrolledSets, entityReachBitmaps) / append(firstDegreeSets, firstDegreeSet) calls to conditionally skip the entityReachBitmaps accumulation and unrolledSets append when handling nodeSlices[0], preserving all exclusion logic that uses ReachSliceOfComponentContainingMember.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/go/analysis/ad/post.go`:
- Line 659: FetchCanRDPData overwrites LocalGroupData.ExcludedShortcutGroups
without updating ExcludedShortcutGroupsSlice, causing divergence used by
CalculateCrossProductNodeSets; fix by ensuring both are updated atomically. In
FetchCanRDPData (the code that calls FetchAuthUsersAndEveryoneGroups and assigns
to LocalGroupData.ExcludedShortcutGroups) also set
LocalGroupData.ExcludedShortcutGroupsSlice =
LocalGroupData.ExcludedShortcutGroups.Slice(), or better add a LocalGroupData
method like SetExcludedShortcutGroups(bitmap) that assigns both
ExcludedShortcutGroups and ExcludedShortcutGroupsSlice, then replace the direct
assignments in FetchLocalGroupData and FetchCanRDPData to call
SetExcludedShortcutGroups(...) so the bitmap and slice remain in sync.
---
Nitpick comments:
In `@packages/go/analysis/ad/ad.go`:
- Around line 568-569: The loop currently builds and appends per-entity
accumulations into entityReachBitmaps for every nodeSlice, but the element at
index 0 of unrolledSets is never read; update the loop so that when iterating
nodeSlices[0] you still call ReachSliceOfComponentContainingMember for the
exclusion check but skip appending to entityReachBitmaps and do not append an
entry to unrolledSets for index 0; keep building firstDegreeSets as before (or
else adjust the later len(firstDegreeSets) checks) and ensure subsequent code
still reads unrolledSets[1] and unrolledSets[2:] as before. Specifically modify
the code around the append(unrolledSets, entityReachBitmaps) /
append(firstDegreeSets, firstDegreeSet) calls to conditionally skip the
entityReachBitmaps accumulation and unrolledSets append when handling
nodeSlices[0], preserving all exclusion logic that uses
ReachSliceOfComponentContainingMember.
urangel
approved these changes
Feb 20, 2026
ALCooper12
approved these changes
Feb 20, 2026
…ip as commutative - BED-7379
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/go/analysis/ad/ad.go`:
- Around line 540-566: The reach-set exclusion loop in
CalculateCrossProductNodeSets assumes LocalGroupData.ExcludedShortcutGroupsSlice
is populated; add a defensive guard at the start of
CalculateCrossProductNodeSets that detects when
localGroupData.ExcludedShortcutGroupsSlice is nil or empty while
localGroupData.ExcludedShortcutGroups is non-empty and then populates
ExcludedShortcutGroupsSlice from ExcludedShortcutGroups (e.g., iterate the
set/map/bitmap in ExcludedShortcutGroups and append keys/IDs into
ExcludedShortcutGroupsSlice) so the subsequent loop over
ExcludedShortcutGroupsSlice behaves correctly even if LocalGroupData was
constructed outside FetchLocalGroupData.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
Description
Reach calculation for AD Group and LocalGroup recursive membership currently merges all memberships into a single bitmap. This operation is both CPU and memory intensive. Usage of reach calculation specifically in
CalculateCrossProductNodeSetswhere this hotspot was located does not require the merge step as all operations performed are commutative.This changeset uses a newer release of
dawgsthat contains updates to the reach cache to avoid the above caveats. The refactoredCalculateCrossProductNodeSetsno longer merges reach membership bitmaps and instead uses commutative operations and an iterative method for validating membership.Motivation and Context
Resolves BED-7379
Lower CPU and memory consumption. Faster. Better.
How Has This Been Tested?
Existing tests both unit and integration cover the refactored codepaths. Additional testing was done in
dawgsfor version v0.4.8.Types of changes
Checklist:
Summary by CodeRabbit
Chores
Improvements