Skip to content

chore: Migrate cross-product lookups to use bitmask isntead of CommutativeDuplex - BED-8362 #2821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
StephenHinck wants to merge 2 commits into
base: main
Choose a base branch
from
+16 −15
Open

chore: Migrate cross-product lookups to use bitmask isntead of CommutativeDuplex - BED-8362 #2821

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d4d373a
Migrate cross-product lookups to use bitmask isntead of CumulativeDuplex
StephenHinck May 19, 2026
438b5df
nit: remove point-in-time comments from robot
StephenHinck May 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions packages/go/analysis/ad/ad.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,12 +524,9 @@ func CalculateCrossProductNodeSets(localGroupData *LocalGroupData, nodeSlices ..
firstDegreeSets []cardinality.Duplex[uint64]
unrolledSets [][]cardinality.Duplex[uint64]

// This is the set we use as a reference set to check against checkset
// This is the set we use as a reference set to check against the materialized check set
unrolledRefSet = cardinality.NewBitmap64()

// This is the set we use to aggregate multiple sets together it should have all the valid principals from all other sets at this point
checkSet = cardinality.CommutativeDuplexes[uint64]{}

// This is our set of entities that have the complete cross product of permissions
resultEntities = cardinality.NewBitmap64()
)
Expand Down Expand Up @@ -610,15 +607,23 @@ func CalculateCrossProductNodeSets(localGroupData *LocalGroupData, nodeSlices ..
}

// 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]...))
Copy link
Copy Markdown
Contributor

@urangel urangel May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@urangel urangel May 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Materialize the check set as a single bitmap by Or-ing all component bitmaps within each set, then And-ing across sets.
Copy link
Copy Markdown
Contributor

@urangel urangel May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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.

materializedCheckSet := cardinality.NewBitmap64()
for _, bm := range unrolledSets[1] {
Copy link
Copy Markdown
Contributor

@urangel urangel May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

materializedCheckSet.Or(bm)
}

for _, unrolledSet := range unrolledSets[2:] {
checkSet.And(cardinality.CommutativeOr(unrolledSet...))
setUnion := cardinality.NewBitmap64()
for _, bm := range unrolledSet {
setUnion.Or(bm)
}
materializedCheckSet.And(setUnion)
}

// Check first degree principals in our reference set (firstDegreeSets[0]) first
firstDegreeSets[0].Each(func(id uint64) bool {
if checkSet.Contains(id) {
if materializedCheckSet.Contains(id) {
resultEntities.Add(id)
} else {
localGroupData.GroupMembershipCache.OrReach(id, graph.DirectionInbound, unrolledRefSet)
Expand Down Expand Up @@ -667,7 +672,7 @@ func CalculateCrossProductNodeSets(localGroupData *LocalGroupData, nodeSlices ..
continue
}

if checkSet.Contains(groupId) {
if materializedCheckSet.Contains(groupId) {
// If this entity is a cross product, add it to result entities, remove the group id from the second set and remove the group's membership from the result set
resultEntities.Add(groupId)

Expand All @@ -687,13 +692,9 @@ func CalculateCrossProductNodeSets(localGroupData *LocalGroupData, nodeSlices ..
}
}

unrolledRefSet.Each(func(remainder uint64) bool {
if checkSet.Contains(remainder) {
resultEntities.Add(remainder)
}

return true
})
// Use bitmap-level intersection to confirm membership in check set.
unrolledRefSet.And(materializedCheckSet)
resultEntities.Or(unrolledRefSet)

return resultEntities
}
Expand Down
Loading