feat(EmitHelper): use cache to reduce dynamic type creation.#7797
Merged
feat(EmitHelper): use cache to reduce dynamic type creation.#7797
Conversation
Contributor
Reviewer's GuideIntroduces a cache-based mechanism for dynamic type creation in DataTableDynamicContext using CacheManager, refactors the data row cache field naming, and slightly adjusts a sample to set column display text consistently via the dynamic context callback. Sequence diagram for cache-based dynamic type creation in DataTableDynamicContextsequenceDiagram
participant DataTableDynamicContext
participant CacheManager
participant CacheInstance
participant EmitHelper
DataTableDynamicContext->>DataTableDynamicContext: Build columnNames and cacheKey
DataTableDynamicContext->>CacheManager: GetDynamicObjectTypeByName(key, cols, OnColumnCreating, out cached)
CacheManager->>CacheInstance: GetOrCreate(key, factory)
alt type not in cache
CacheInstance->>CacheManager: invoke factory
CacheManager->>EmitHelper: CreateTypeByName(dynamicTypeName, cols, DataTableDynamicObject, creatingCallback)
EmitHelper-->>CacheManager: Type
CacheManager-->>CacheInstance: created Type
CacheInstance-->>CacheManager: Type
CacheManager-->>DataTableDynamicContext: Type (cached = false)
DataTableDynamicContext->>DataTableDynamicContext: Use cols with OnColumnCreating
else type in cache
CacheInstance-->>CacheManager: cached Type
CacheManager-->>DataTableDynamicContext: Type (cached = true)
DataTableDynamicContext->>DataTableDynamicContext: if AddAttributesCallback != null
DataTableDynamicContext->>DataTableDynamicContext: foreach col in cols
DataTableDynamicContext->>DataTableDynamicContext: AddAttributesCallback(this, col)
end
DataTableDynamicContext->>DataTableDynamicContext: return dynamicType
Updated class diagram for DataTableDynamicContext and CacheManager dynamic type cachingclassDiagram
class DataTableDynamicContext {
-ConcurrentDictionary~Guid, (IDynamicObject, DataRow)~ _dataCache
-DataTable DataTable
-Action~DataTableDynamicContext, ITableColumn~ AddAttributesCallback
+DataTableDynamicContext(DataTable table, Action~DataTableDynamicContext, ITableColumn~ callback)
+IEnumerable~IDynamicObject~ GetItems()
+Task AddAsync(IEnumerable~IDynamicObject~ selectedItems)
+Task~bool~ DeleteAsync(IEnumerable~IDynamicObject~ items)
-Task OnCellValueChanged(IDynamicObject item, ITableColumn column, object val)
-List~IDynamicObject~ BuildItems()
-Type CreateType()
}
class CacheManager {
<<static>>
+Type GetDynamicObjectTypeByName(string key, IEnumerable~ITableColumn~ cols, Func~ITableColumn, IEnumerable~CustomAttributeBuilder~~ creatingCallback, out bool cached)
-T GetOrCreate~T~(string key, Func~string, T~ factory)
}
class DataTableDynamicObject {
Guid DynamicObjectPrimaryKey
DataRow Row
}
class ITableColumn {
string Text
string GetFieldName()
}
class DataRow
class DataTable
class CustomAttributeBuilder
DataTableDynamicContext ..> CacheManager : uses
DataTableDynamicContext ..> DataTableDynamicObject : creates
DataTableDynamicContext ..> ITableColumn : configures
DataTableDynamicContext ..> DataRow : maps
CacheManager ..> CustomAttributeBuilder : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
GetDynamicObjectTypeByName, thecreatedflag is mutated inside the cache factory delegate and then read afterward, which is racy if the cache implementation can call the factory more than once or across threads; consider deriving thecachedvalue directly from the cache API (e.g., a TryGet/added flag) instead of relying on a captured local. - The cache key for dynamic types currently concatenates column name and full type name with
|; if column ordering is not guaranteed or additional properties are added later, you may unintentionally create multiple dynamic types for logically equivalent schemas—consider normalizing/sorting or using a more structured key to avoid redundant type generation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GetDynamicObjectTypeByName`, the `created` flag is mutated inside the cache factory delegate and then read afterward, which is racy if the cache implementation can call the factory more than once or across threads; consider deriving the `cached` value directly from the cache API (e.g., a TryGet/added flag) instead of relying on a captured local.
- The cache key for dynamic types currently concatenates column name and full type name with `|`; if column ordering is not guaranteed or additional properties are added later, you may unintentionally create multiple dynamic types for logically equivalent schemas—consider normalizing/sorting or using a more structured key to avoid redundant type generation.
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor/Dynamic/DataTableDynamicContext.cs" line_range="43" />
<code_context>
/// <para lang="en">Responsible for associating DataRow with Items to facilitate lookup and improve efficiency</para>
/// </summary>
- private ConcurrentDictionary<Guid, (IDynamicObject DynamicObject, DataRow Row)> Caches { get; } = new();
+ private ConcurrentDictionary<Guid, (IDynamicObject DynamicObject, DataRow Row)> _dataCache = new();
/// <summary>
</code_context>
<issue_to_address>
**suggestion:** Consider making the `_dataCache` field `readonly` to avoid accidental reassignment.
This was previously an auto-property with an initializer, so the dictionary reference itself wasn’t reassignable. As a mutable field, `_dataCache` can now be replaced, which doesn’t seem necessary and increases the risk of accidental reassignment. Making it `readonly` keeps the reference fixed while still allowing normal dictionary operations.
```suggestion
private readonly ConcurrentDictionary<Guid, (IDynamicObject DynamicObject, DataRow Row)> _dataCache = new();
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7797 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 764 764
Lines 34111 34122 +11
Branches 4697 4697
=========================================
+ Hits 34111 34122 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces caching for EmitHelper-generated dynamic types used by DataTableDynamicContext, aiming to reduce repeated Reflection.Emit type creation and improve runtime efficiency when constructing dynamic table contexts.
Changes:
- Added a new
CacheManagerhelper to cache and reuse emitted dynamicTypeinstances by a computed key. - Updated
DataTableDynamicContextto generate a schema-based cache key and reuse cached dynamic types. - Adjusted the server sample to demonstrate setting column display text via
ITableColumn.Text.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/BootstrapBlazor/Services/CacheManager.cs | Adds an internal API to cache/retrieve emitted dynamic Types and report cache hits. |
| src/BootstrapBlazor/Dynamic/DataTableDynamicContext.cs | Switches dynamic type creation to use the new cache, and renames internal row/object cache field. |
| src/BootstrapBlazor.Server/Components/Samples/Table/TablesDynamic.razor.cs | Updates sample code to set col.Text earlier as a display-name example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+93
to
+97
| // Emit 生成动态类 (使用缓存) | ||
| var columnNames = string.Join('|', table.Columns.Cast<DataColumn>().Select(static c => $"{c.ColumnName}:{c.DataType.FullName}")); | ||
| var cacheKey = $"BootstrapBlazor-{nameof(DataTableDynamicContext)}-{columnNames}"; | ||
| var dynamicType = CacheManager.GetDynamicObjectTypeByName(cacheKey, cols, OnColumnCreating, out var cached); | ||
|
|
There was a problem hiding this comment.
The dynamic type cache key only includes the DataTable schema (column names/types), but the emitted type also depends on AddAttributesCallback/OnColumnCreating (e.g., required/error messages, display attributes, ignore settings). If two contexts share the same schema but provide different callbacks/localizers, the second context will reuse the first type and its baked-in attributes, causing incorrect column metadata/validation (this is already reproducible in TablesDynamic.razor.cs, where two contexts use different localizers with the same schema). Consider incorporating callback identity and/or CultureInfo.CurrentUICulture into the cache key, or disabling type caching when attribute generation is callback-dependent.
Comment on lines
+773
to
+776
| { | ||
| created = true; | ||
| return EmitHelper.CreateTypeByName($"BootstrapBlazor_{nameof(DataTableDynamicContext)}_{key}", cols, typeof(DataTableDynamicObject), creatingCallback); | ||
| }); |
There was a problem hiding this comment.
The emitted type name is built from key ($"BootstrapBlazor_{nameof(DataTableDynamicContext)}_{key}"), but the cache key can become very long and may contain characters from column names (e.g., |, :, spaces) and generic type full names. This can bloat metadata and risks DefineType failures for large schemas. Consider generating a short/sanitized type name (e.g., a stable hash of the schema + any other discriminators) while keeping the full string only as the cache key.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Link issues
fixes #7788
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Introduce caching for dynamically emitted DataTable types and streamline dynamic table context usage.
New Features:
Enhancements: