Skip to content

Improve batch state checker performance and add RequireFeaturesSimpleBatchStateChecker#25276

Open
maliming wants to merge 5 commits intorel-10.3from
improve/batch-state-checker-shared-scope
Open

Improve batch state checker performance and add RequireFeaturesSimpleBatchStateChecker#25276
maliming wants to merge 5 commits intorel-10.3from
improve/batch-state-checker-shared-scope

Conversation

@maliming
Copy link
Copy Markdown
Member

@maliming maliming commented Apr 15, 2026

When IsEnabledAsync(TState[]) evaluates non-batch checkers, the original implementation called InternalIsEnabledAsync for each state, creating a new DI scope every time. With thousands of permissions (e.g. 4050 permissions each with RequireFeaturesSimpleStateChecker), this caused N scope creations and N redundant Redis/service queries (~3.5s).

SimpleStateCheckerManager changes:

  • Batch path: shares one DI scope across all non-batch checker evaluations via AsyncLocal, creates a new CachedServiceProvider per state to prevent transient service leakage across states.
  • Single-state path: unchanged, still creates its own scope.
  • Subclass overrides of InternalIsEnabledAsync are fully respected in both paths.

RequireFeaturesSimpleBatchStateChecker (new):

Follows the same pattern as RequirePermissionsSimpleBatchStateChecker:

  • Pre-loads all unique feature values at once (~30 Redis calls instead of 4000+)
  • Evaluates all states against in-memory data
  • RequireFeatures() extension defaults to batchCheck: true
  • Non-batch path preserved via batchCheck: false parameter
  • MenuManager and ToolbarManager use Use(new ...) to reset per request

IFeatureChecker:

  • Added IsEnabledAsync(string[] names) batch method to IFeatureChecker
  • Default implementation in FeatureCheckerBase

Measured improvement: 3519ms → 41ms for 4050 permissions with feature state checkers.

When IsEnabledAsync(TState[]) evaluates non-batch checkers, the original
implementation called InternalIsEnabledAsync for each state, which created
a new DI scope every time. In real-world scenarios with thousands of
permissions (e.g. 4050 permissions each with RequireFeaturesSimpleStateChecker),
this caused N scope creations and N redundant Redis queries, resulting in
~3.5s latency.

This change shares a single DI scope across all non-batch checker evaluations
in the batch path by extracting EvaluateCheckersAsync and calling it directly
with the shared scope. Each state still gets an isolated ITransientCachedServiceProvider
to prevent transient service leakage across states, while scoped services
(e.g. IFeatureChecker) are naturally shared within the scope, enabling cache reuse.

The single-state path (IsEnabledAsync(TState)) remains completely unchanged.
Copilot AI review requested due to automatic review settings April 15, 2026 09:34
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Improves SimpleStateCheckerManager<TState>.IsEnabledAsync(TState[]) performance by avoiding per-state DI scope creation when evaluating non-batch checkers, while adding regression tests to validate correctness and scope/cached-provider isolation behavior.

Changes:

  • Reuses the existing batch scope when evaluating non-batch checkers by extracting EvaluateCheckersAsync.
  • Uses ITransientCachedServiceProvider per state in the batch/non-batch path to isolate transient caching across states.
  • Adds new tests covering correctness, skip behavior, and scope isolation (including nested single-state calls).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
framework/src/Volo.Abp.Core/Volo/Abp/SimpleStateChecking/SimpleStateCheckerManager.cs Refactors evaluation logic to reuse one scope in the batch path and introduces EvaluateCheckersAsync.
framework/test/Volo.Abp.Core.Tests/Volo/Abp/SimpleStateChecking/SimpleStateChecker_ScopeIsolation_Tests.cs Adds probe-based tests validating shared batch scope + per-state cached provider isolation and nested-call scope behavior.
framework/test/Volo.Abp.Core.Tests/Volo/Abp/SimpleStateChecking/SimpleStateChecker_BatchSingleScope_Tests.cs Adds functional tests ensuring batch evaluation of non-batch checkers remains correct after scope reuse.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@maliming maliming added this to the 10.3-patch milestone Apr 15, 2026
@maliming maliming marked this pull request as draft April 15, 2026 12:37
@maliming maliming changed the title Improve SimpleStateCheckerManager batch performance by sharing a single DI scope Improve batch state checker performance and add RequireFeaturesSimpleBatchStateChecker Apr 16, 2026
@maliming maliming requested a review from Copilot April 16, 2026 01:18
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

Replace linear scans with Dictionary/HashSet lookups in both
RequirePermissionsSimpleBatchStateChecker and
RequireFeaturesSimpleBatchStateChecker for consistency.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

@maliming maliming marked this pull request as ready for review April 16, 2026 01:53
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 16, 2026

Codecov Report

❌ Patch coverage is 41.31148% with 179 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.70%. Comparing base (040a1e7) to head (6505dc0).
⚠️ Report is 50 commits behind head on rel-10.3.

Files with missing lines Patch % Lines
...hecking/SimpleStateChecker_ScopeIsolation_Tests.cs 0.00% 96 Missing ⚠️
...cking/SimpleStateChecker_BatchSingleScope_Tests.cs 0.00% 72 Missing ⚠️
...bp/Features/FeatureSimpleStateCheckerExtensions.cs 57.14% 5 Missing and 1 partial ⚠️
...sions/RequirePermissionsSimpleBatchStateChecker.cs 84.21% 2 Missing and 1 partial ⚠️
...Management/PermissionDefinitionSerializer_Tests.cs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           rel-10.3   #25276      +/-   ##
============================================
- Coverage     50.71%   50.70%   -0.02%     
============================================
  Files          3472     3477       +5     
  Lines        117140   117489     +349     
  Branches       8840     8866      +26     
============================================
+ Hits          59407    59568     +161     
- Misses        55925    56110     +185     
- Partials       1808     1811       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@maliming maliming requested a review from EngincanV April 16, 2026 02:39
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