Improve performance of XML doc inference #8794
Open
Conversation
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
Worse for inheritdoc than before ChilliCream#8775
|
Full benchmark (values basically scale linearly)
|
N-Olbert
commented
Oct 10, 2025
| .GetParameters() | ||
| .Single(p => p.Name == "baz")); | ||
|
|
||
| parameterXml = documentationProvider.GetDescription( |
N-Olbert
commented
Oct 10, 2025
| @@ -0,0 +1,1260 @@ | |||
| using System.Collections.Concurrent; | |||
N-Olbert
commented
Oct 10, 2025
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="BenchmarkDotNet" /> |
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.
Summary of the changes (Less than 80 chars)
Related to #8775 (in this specific format)
As discussed in #8775, the current implementation was quite slow.
I intentionally did not introduce any big conceptual changes (yet) and focused only on optimizing the current implementation.
According to the benchmarks below, the general performance has improved by a factor between 2 and 22 (without the
inheritdoc-cases, see below for them) - on average around 3.5× faster - while memory usage has been reduced significantly to around a third.For
inheritdoc-cases, the situation is a bit different: I discovered that the current implementation mutates the XElement instances retrieved from the XmlDocumentationFileResolver, effectively modifying its internal cache.Here :
graphql-platform/src/HotChocolate/Core/src/Types/Types/Descriptors/Conventions/XmlDocumentationProvider.cs
Lines 320 to 321 in dfe075c
and here:
graphql-platform/src/HotChocolate/Core/src/Types/Types/Descriptors/Conventions/XmlDocumentationProvider.cs
Lines 353 to 354 in dfe075c
This behavior is both undesired (at least I think so) and not thread-safe.
To fix this, I changed the logic to create a copy of the XElement when resolving inheritdoc elements.
This ensures that the cache remains intact and that the implementation is thread-safe.
While this introduces a small performance regression for two out of six inheritdoc cases, I think it is justified since the previous implementation was incorrect and should not be considered a valid baseline: It only replaced the inheritdoc element once within the cached(!) XDocument, meaning that all subsequent calls for the same(!!!) member (which happens all the time in the benchmark) did not need to replace it again and threfore resulting in slightly better, but ultimately incomparable, performance characteristics. Of course, the current behavior could indeed be beneficial at runtime as well, f. e. an interface with many inheritors, but it is not that common as the benchmark indicates.
Overall, even for the
inheritdoc-cases, the performance is still improved or comparable to before, and the memory usage is significantly lower:Note: The PR currently includes the benchmark code so that one can reproduce the results.
I would to remove these benchmarks after the review and applying any suggested change.
Note further: This is a breaking change since I reworked
XmlDocumentationFileResolverto becomeXmlDocumentationResolver.This change is not strictly neccessary, but IIRC it was a 10-20% speed boost - I will reevaluate the concret impact on request if the change seems unlikely to be acceptable.
Full benchmarks: