Skip to content

ROCK-8706 follow-up: remove the three LoadHosts hot operators (guest counts, occurrence N+1, employee scan) - #263

Merged
gmcgrady merged 4 commits into
masterfrom
ROCK-8706-loadhosts-guestcount-batch
Jul 14, 2026
Merged

ROCK-8706 follow-up: remove the three LoadHosts hot operators (guest counts, occurrence N+1, employee scan)#263
gmcgrady merged 4 commits into
masterfrom
ROCK-8706-loadhosts-guestcount-batch

Conversation

@jwakefield-secc

@jwakefield-secc jwakefield-secc commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

TL;DR — how the three fixes relate

All three are variations on one theme: EF6 doing per-attendance-row work over a huge table. But they split into a cousin and a matched pair:

What EF6 did wrong The fix
#262 (already merged) Occurrence filter applied after the GroupJoin, so EF deferred it and ran the member/employee OR over the full 9.4M-row Attendance table Reorder — narrow by occurrence first
Fix B (guests) Guest count compiled into a correlated per-row subquery (the ValueAsPersonId UDF, re-run for every row) Materialize once into a dictionary
Fix C (employees) Employee set (IQueryable.Contains) compiled into a correlated per-row EXISTS scanning attr-740 Materialize once into an IN-list

Fix B and Fix C are the same bug on two different columns — a correlated subquery that should be a one-time set operation, fixed the same way. #262 was a cousin (a filter-ordering problem, fixed by reordering — not by materializing).

Why three rounds instead of one: #262 removed the full-table scan (the post-#262 plan confirms Attendance is now reached by an IX_OccurrenceId seek), but the two correlated subqueries were underneath it the whole time and became the top of the plan. Note: there's no clean before/after timing for #262 — the pre-#262 statement has aged out of prod Query Store — so this ordering is plan-based, not a measured delta. #262 fixed a real cost; it just wasn't the floor that B and C set, which is why it didn't visibly move the wall-clock. One-liner: #262 stopped scanning the whole table; B and C stop re-running a subquery once per row — the guest half and the employee half of the same remaining problem.


Follow-up to #262 (merged). #262 reordered the attendance filters and added AsNoTracking(), which killed the 10–15s member/employee full-table scan. Pulling the actual Query Store execution plan for the surviving 28.7s statement (query_id 29156590) then showed the LoadHosts host-list query still had three hot operators — this PR removes all three.

What the plan showed

For the Project12 host-list query (returns Host = PersonAlias.Person + guest count):

  • NonParallelPlanReason="TSQLUserDefinedFunctionsNotParallelizable" — the whole plan was forced single-threaded by the ufnUtility_GetPersonIdFromPersonAliasGuid scalar UDF.
  • StatementOptmEarlyAbortReason="TimeOut" — the query was too complex for the optimizer to plan well; cardinality was badly off (1 estimated vs ~185 actual).
  • Dominant operator: a correlated AttributeValue scan for AttributeId=740 (~11k rows) rewound per attendance row ≈ 2M row reads, subtree cost 2.42 of 3.01.

Fix A — batch the occurrence lookup (N+1 → 1)

LoadHosts() called AttendanceOccurrenceService.Get() once per active (group, location, schedule) triple. Collapsed into a single query intersected against the exact triple set in memory. Safe because Get(DateTime,int?,int?,int?) is read-only (Queryable().FirstOrDefault(), verified against Rock 1.13.7 source) and IX_GroupId_LocationID_ScheduleID_Date is UNIQUE, so "all matching" ≡ old per-triple FirstOrDefault().

Fix B — decouple the guest-count aggregation

The guest count was folded into the attendance query via GroupJoin; EF6 emitted it as two correlated TOP 1 subqueries over the ValueAsPersonId scalar UDF, rebound per attendance row. Now runs as its own query materialized into a Dictionary<int,int> (~87ms on DEV) and stitched onto each host after the attendance query materializes. This also removes the UDF from the query → unlocks parallelism and simplifies it enough to stop the optimizer timeout.

Fix C — decouple the employee lookup

The member/employee OR left employees.Contains(PersonId) as an IQueryable subquery → the correlated AttributeId=740 scan above. AttributeValue.Value is nvarchar(max) so it can't be an index key (an index can't turn this into a seek). Materializing the ~676 employee person ids once turns the OR into a constant IN-list evaluated a single time, removing the ~2M-row rescan.

Equivalence

Behavior-preserving across all three: same attendance filters, same OrderBy/ThenBy, same per-host guest counts, same employee id set (same AttributeId=740 + Value predicate; null EntityIds filtered — a null could never match a non-null PersonId). RowDataBound's max-guest logic reads GuestCount exactly as before.

Verified on prod/DEV data (read-only):

  • Fix A uniqueness invariant — 0 violating rows over 60 days.
  • Fix B guest-count equivalence — old correlated-subquery form vs new GROUP BY form diffed to 0 rows, including a busy day (2025-12-30: 46 hosts, 24 of them multi-guest).
  • The 28.7s statement confirmed as this LoadHosts query (not LoadPendingCheckins).

jwakefield-secc and others added 2 commits July 9, 2026 13:31
Fix B (guest-count tail): the guest-count aggregation was folded into the
attendance query via GroupJoin, which EF6 rewrote as a per-attendance-row
correlated subquery over the ValueAsPersonId scalar UDF — tail-spiking to
28.7s in Query Store. Run it as its own query materialized into a
Dictionary<int,int> (~87ms on DEV) and stitch GuestCount onto each host
after the attendance query materializes.

Fix A (occurrence N+1): the active-occurrence lookup made one
AttendanceOccurrenceService.Get() round trip per active (group, location,
schedule) triple. That Get(DateTime,int?,int?,int?) overload is read-only
(Queryable().FirstOrDefault(), no create/save) and
IX_GroupId_LocationID_ScheduleID_Date is UNIQUE on
(GroupId, LocationId, ScheduleId, OccurrenceDate), so the loop collapses to
a single query (three IN-lists) intersected against the exact triple set in
memory — provably identical result, one round trip instead of N.

Behavior-preserving: same attendance filters, same ordering, same per-host
guest counts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The member/employee OR left "employees.Contains(PersonId)" as an IQueryable
subquery, which EF6 compiled into a correlated EXISTS re-evaluated per
attendance row. AttributeValue.Value is nvarchar(max) (not indexable), so
each evaluation seeked attribute 740 and residual-filtered the string over
~11k rows -> ~2M row reads, the single largest operator in the Query Store
plan (subtree cost 2.42 of 3.01). The scalar UDF in the guest-count subquery
also forced the whole plan single-threaded (TSQLUserDefinedFunctionsNot-
Parallelizable) and the optimizer timed out.

Materialize the ~676 employee person ids ONCE into a list so the OR becomes a
constant IN-list evaluated a single time. Combined with Fix B (guest-count
decoupling, which removes the UDF and unlocks parallelism) the remaining
attendance query is a clean occurrence seek -> person lookup.

Behavior-preserving: same employee id set (same AttributeId=740 + Value
predicate), null EntityIds filtered (a null could never match a non-null
PersonId), OR unchanged -> identical host rows.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jwakefield-secc jwakefield-secc changed the title ROCK-8706 follow-up: decouple guest counts + batch occurrence lookup in LoadHosts ROCK-8706 follow-up: remove the three LoadHosts hot operators (guest counts, occurrence N+1, employee scan) Jul 9, 2026
jwakefield-secc and others added 2 commits July 10, 2026 14:22
The Fix B guest-count stitch loop used an inline out-variable declaration
(`out int guestCount`), a C# 7 feature. RockWeb compiles plugin .ascx.cs
code-behind at runtime under C# 6, which rejects it (CS8059: "Feature 'out
variable declaration' is not available in C# 6"), so the block failed to
load. Declare guestCount before the loop and pass `out guestCount`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fix A built activeGroupIds/activeLocationIds/activeScheduleIds as HashSet<int>
and used .Contains() inside the EF6 IQueryable. EF6 translates List<T>.Contains
to SQL IN but does NOT reliably translate HashSet<T>.Contains (throws
NotSupportedException at query execution — a runtime page-load failure, not a
compile error). Materialize the three to List<int> before the query (dedup is
still done by the HashSet build). Consistent with the List used for employeeIds
in Fix C. The activeTriples HashSet is unaffected — it's used after ToList()
(in-memory LINQ), not translated to SQL.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jwakefield-secc
jwakefield-secc requested a review from gmcgrady July 13, 2026 14:18
activeTriples.Add( item.GroupId + "|" + item.LocationId + "|" + schedule.Id );
activeGroupIds.Add( item.GroupId );
activeLocationIds.Add( item.LocationId );
activeScheduleIds.Add( schedule.Id );

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Setting up our in-memory HashSets to build a single query later in lieu of multiple round trips via occurrenceService.Get() -- great choice to avoid unnecessary trips to the DB.


var hostsQry = attendanceQry
.GroupJoin( hostsGuests, a => a.PersonAlias.PersonId, h => h.PersonId,
( a, h ) => new { Attendance = a, GuestCount = h.Select( h1 => h1.GuestCount ).DefaultIfEmpty() } )

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wise to build into dictionary prior to this point, eliminating need to .GroupJoin() in this LINQ query.

@gmcgrady gmcgrady left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Solid pattern of reducing round trips & query complexity by instantiating join-related information in-memory. Assuming tests are clean, this should be good to merge into SEDEV. Approving & merging!

@gmcgrady
gmcgrady merged commit a451d58 into master Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants