Improve surrogate (serialization) performance#589
Closed
Daniel-Svensson wants to merge 1 commit into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughDomainServiceSerializationSurrogate now uses a thread-safe ConcurrentDictionary to cache surrogate type and factory lookups, improving serialization performance by reducing lookup overhead. Supporting deserialization methods and a new public accessor API are added. Changelog documents the 5.9.0 performance improvement. ChangesSurrogate caching and serialization performance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Changelog.md`:
- Line 36: Update the Changelog entry sentence to correct grammar (change
"affect serialization performance" to "affecting serialization performance" or
"impact serialization performance") and add a new bullet noting the public API
addition: mention the new method TryGetSurrogateFactory(Type type) that exposes
cached surrogate factory retrieval so consumers are aware of the API change and
its purpose.
In
`@src/OpenRiaServices.Hosting.Wcf/Framework/Wcf/DomainServiceSerializationSurrogate.cs`:
- Line 89: Fix the typo in the inline comment inside
DomainServiceSerializationSurrogate: change "heirarchy" to the correct spelling
"hierarchy" in the comment that reads "If the type is not found, check the
inheritance heirarchy for a known type." so the comment becomes "If the type is
not found, check the inheritance hierarchy for a known type."
- Around line 104-111: Add an XML documentation comment to the public method
TryGetSurrogateFactory describing its purpose, the type parameter, and the
return value; specifically include a <summary> that explains it attempts to
retrieve a surrogate factory for a given exposed type from the internal cache, a
<param name="type"> describing the expected Type input, a <returns> that states
it returns the surrogate factory delegate when found or null when no mapping
exists, and a brief <remarks> noting that the returned Func<object,object>
produces surrogate instances and callers should handle a null result. Target the
TryGetSurrogateFactory(Type type) method in DomainServiceSerializationSurrogate.
- Around line 72-76: Add an XML documentation comment above the internal method
GetDeserializedObjectFast in DomainServiceSerializationSurrogate: include a
<summary> that describes the fast path behavior for deserializing
surrogate-typed objects, a <param name="obj"> describing the input object
expected to implement ICloneable, and a <returns> describing that the
cloned/deserialized object is returned; place the comment immediately above the
method declaration to comply with the internal API documentation guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2302b97e-f8c5-45aa-a65b-29e9e21e1099
📒 Files selected for processing (2)
Changelog.mdsrc/OpenRiaServices.Hosting.Wcf/Framework/Wcf/DomainServiceSerializationSurrogate.cs
|
|
||
| ## Server | ||
|
|
||
| * Improve performance for creating surrogate types, by reducing number of lookups (affect serialization performance) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Fix grammar and consider documenting the new public API.
Minor grammar issue: "affect serialization performance" should use the participle "affecting" or the verb "impacts" for better readability.
Additionally, the related code changes introduce a new public method TryGetSurrogateFactory(Type type) that exposes cached surrogate factory retrieval. Consider adding a bullet point documenting this new API, as public API additions are typically notable changelog entries.
📝 Suggested revision
-* Improve performance for creating surrogate types, by reducing number of lookups (affect serialization performance)
+* Improve performance for creating surrogate types, by reducing number of lookups (impacting serialization performance)
+* Add public API `TryGetSurrogateFactory(Type type)` to retrieve cached surrogate factory delegates🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Changelog.md` at line 36, Update the Changelog entry sentence to correct
grammar (change "affect serialization performance" to "affecting serialization
performance" or "impact serialization performance") and add a new bullet noting
the public API addition: mention the new method TryGetSurrogateFactory(Type
type) that exposes cached surrogate factory retrieval so consumers are aware of
the API change and its purpose.
Comment on lines
+72
to
+76
| // Fast path to get deserialized object when we know the type is a surrogate, to avoid the dictionary lookup in GetDeserializedObject. | ||
| internal object GetDeserializedObjectFast(object obj) | ||
| { | ||
| return ((ICloneable)obj).Clone(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Add XML documentation comment for new internal API.
Per coding guidelines, new internal APIs should have XML documentation comments. The inline comment explains the intent well, but converting it to XML format would be more consistent.
📝 Suggested XML documentation
- // Fast path to get deserialized object when we know the type is a surrogate, to avoid the dictionary lookup in GetDeserializedObject.
+ /// <summary>
+ /// Fast path to get deserialized object when the type is known to be a surrogate,
+ /// avoiding the dictionary lookup in <see cref="GetDeserializedObject"/>.
+ /// </summary>
+ /// <param name="obj">The surrogate object to clone.</param>
+ /// <returns>The cloned original entity or complex type instance.</returns>
internal object GetDeserializedObjectFast(object obj)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Fast path to get deserialized object when we know the type is a surrogate, to avoid the dictionary lookup in GetDeserializedObject. | |
| internal object GetDeserializedObjectFast(object obj) | |
| { | |
| return ((ICloneable)obj).Clone(); | |
| } | |
| /// <summary> | |
| /// Fast path to get deserialized object when the type is known to be a surrogate, | |
| /// avoiding the dictionary lookup in <see cref="GetDeserializedObject"/>. | |
| /// </summary> | |
| /// <param name="obj">The surrogate object to clone.</param> | |
| /// <returns>The cloned original entity or complex type instance.</returns> | |
| internal object GetDeserializedObjectFast(object obj) | |
| { | |
| return ((ICloneable)obj).Clone(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/OpenRiaServices.Hosting.Wcf/Framework/Wcf/DomainServiceSerializationSurrogate.cs`
around lines 72 - 76, Add an XML documentation comment above the internal method
GetDeserializedObjectFast in DomainServiceSerializationSurrogate: include a
<summary> that describes the fast path behavior for deserializing
surrogate-typed objects, a <param name="obj"> describing the input object
expected to implement ICloneable, and a <returns> describing that the
cloned/deserialized object is returned; place the comment immediately above the
method declaration to comply with the internal API documentation guideline.
Source: Coding guidelines
| var (_, surrogateFactory) = this._exposedTypeToSurrogateMap.GetOrAdd(type, static (key, This) => | ||
| { | ||
| if (this.exposedTypeToSurrogateMap.TryGetValue(exposedType, out var surrogateInfo)) | ||
| // If the type is not found, check the inheritance heirarchy for a known type. |
Contributor
There was a problem hiding this comment.
Fix typo: "heirarchy" → "hierarchy".
- // If the type is not found, check the inheritance heirarchy for a known type.
+ // If the type is not found, check the inheritance hierarchy for a known type.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // If the type is not found, check the inheritance heirarchy for a known type. | |
| // If the type is not found, check the inheritance hierarchy for a known type. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/OpenRiaServices.Hosting.Wcf/Framework/Wcf/DomainServiceSerializationSurrogate.cs`
at line 89, Fix the typo in the inline comment inside
DomainServiceSerializationSurrogate: change "heirarchy" to the correct spelling
"hierarchy" in the comment that reads "If the type is not found, check the
inheritance heirarchy for a known type." so the comment becomes "If the type is
not found, check the inheritance hierarchy for a known type."
Comment on lines
+104
to
111
| public Func<object, object> TryGetSurrogateFactory(Type type) | ||
| { | ||
| if (this._exposedTypeToSurrogateMap.TryGetValue(type, out var surrogateInfo)) | ||
| { | ||
| return surrogateInfo.surrogateFactory; | ||
| } | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
Add XML documentation comment for new public API.
Per coding guidelines, new public APIs should have XML documentation comments. This method exposes internal caching to consumers, so documenting its purpose, parameters, and return value is important.
📝 Suggested XML documentation
+ /// <summary>
+ /// Gets the surrogate factory delegate for the specified type, if available.
+ /// </summary>
+ /// <param name="type">The exposed CLR type to look up.</param>
+ /// <returns>
+ /// A factory delegate that creates a surrogate instance from an entity or complex type instance,
+ /// or <see langword="null"/> if no surrogate is registered for the specified type.
+ /// </returns>
public Func<object, object> TryGetSurrogateFactory(Type type)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/OpenRiaServices.Hosting.Wcf/Framework/Wcf/DomainServiceSerializationSurrogate.cs`
around lines 104 - 111, Add an XML documentation comment to the public method
TryGetSurrogateFactory describing its purpose, the type parameter, and the
return value; specifically include a <summary> that explains it attempts to
retrieve a surrogate factory for a given exposed type from the internal cache, a
<param name="type"> describing the expected Type input, a <returns> that states
it returns the surrogate factory delegate when found or null when no mapping
exists, and a brief <remarks> noting that the returned Func<object,object>
produces surrogate instances and callers should handle a null result. Target the
TryGetSurrogateFactory(Type type) method in DomainServiceSerializationSurrogate.
Source: Coding guidelines
Member
Author
|
no measurable perf change for binary serialization. |
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
1 participant
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.
This pull request focuses on improving the performance of surrogate type creation in the WCF server serialization logic, which should positively impact serialization performance. The main changes involve optimizing how surrogate types and their factories are stored and accessed, as well as simplifying some of the logic for handling these types.
Documentation updates:
Summary by CodeRabbit
Performance Improvements
Release