Non-recursive TypeName.FullName implementation #112242
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-reflection-metadata |
|
The problem we have talked about during review of this feature was non-linear amount of work and allocations performed by FullName. I do not think that the recursive implementation is a problem. The recursion has the same approximate depth as the recursion that was performed during parsing of the type name, so the recursion here does not introduce a new problem. I agree that the non-recursive implementation makes the code complicated. |
There was a problem hiding this comment.
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs:204
- Ensure that the assembly name is not appended multiple times by adding a check before appending.
AppendInReverseOrder(ref builder, current.AssemblyName.FullName.AsSpan());
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs
Show resolved
Hide resolved
| else if (_nestedNameLength > 0 && _fullName.Length > _nestedNameLength) // Declaring types | ||
|
|
||
| Span<char> characters = builder.RawChars.Slice(0, builder.Length); | ||
| characters.Reverse(); |
There was a problem hiding this comment.
Consider using a more efficient method to reverse the string if performance is a concern.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| #endif | ||
| // Stored fullName represents the full name of the nested type. | ||
| // Example: Namespace.Declaring+Nested | ||
| _fullName = _fullName.Substring(0, _nestedNameLength); |
There was a problem hiding this comment.
How about thread safety of this? Concurrent initialization with the same content should be safe, but this branch is modifying to different content.
There was a problem hiding this comment.
TypeName is not thread safe. In worst case, multiple different threads are going to perform a string allocation.
There was a problem hiding this comment.
TypeNameis not thread safe.
Isn't it? It's immutable. There might be some extra allocations but it would work fine when accessed from multiple threads, wouldn't it?
There was a problem hiding this comment.
You are right, it's fine to assume that something that is immutable is also thread-safe.
And again, in worst case, multiple threads would allocate the same string and duplicate the work, but each of them would end up computing the same text.
| while (stack.Count > 0) | ||
| { | ||
| NameInfo currentInfo = stack[stack.Count - 1]; | ||
| stack.RemoveAt(stack.Count - 1); |
There was a problem hiding this comment.
Decrease stack.Count? Otherwise it should be infinite loop.
There was a problem hiding this comment.
RemoveAt takes care of decreasing the Count
| _fullName = characters.ToString(); | ||
| builder.Dispose(); |
There was a problem hiding this comment.
ToString already calls Dispose
There was a problem hiding this comment.
But in this case I am not calling builder.ToString(), but builder.RawChars.ToString which does not call the Dispose
|
Closing in favor of #112350 |
The simple recursive implementation turned into quite a complex iterative approach. I am open to any suggestions to make it better.
I've not touched the "Name" implementation yet, as I want to avoid a merge conflict with #111598 and get a confirmation about the direction I took