perf: ObjectGraph remove unused fields and methods#4415
perf: ObjectGraph remove unused fields and methods#4415thomhurst merged 1 commit intothomhurst:mainfrom
ObjectGraph remove unused fields and methods#4415Conversation
SummaryThis PR removes unused fields and methods from Critical Issues1. Thread Safety Concern - Potential Data RaceLocation: The PR removes locking from Before: lock (objects)
{
return objects.ToArray();
}After: return objects;Issue: The Risk: If
Required Fix: Either:
2. Missing Parameter in Constructor CallsLocation: The PR removes the Issue: This creates unnecessary allocations and lock contention for data that's immediately discarded. Suggestion: Since Suggestions3. Struct Default Constructor PitfallMaking Mitigation: Consider adding null checks in 4. Return Type Changed from IEnumerable to IReadOnlyCollectionLocation:
Verify: Confirm this doesn't break reflection-based consumers or serialization. Previous Review StatusUnable to retrieve previous comments due to API scope limitations. Verdict |
Correct, I'm working on a different PR to remove this and possibly make
|
SummaryThis PR removes unused fields and methods from ObjectGraph, converts it from a class to a struct, and removes the IObjectGraph interface. Critical Issues1. Struct containing mutable reference type violates best practicesIn internal readonly struct ObjectGraph
{
private readonly ConcurrentDictionary<int, HashSet<object>> _objectsByDepth;
// ...
}Problem: Structs should generally not contain mutable reference types like Why this matters:
Recommendation: Keep ObjectGraph as a 2. Thread safety concern in GetObjectsAtDepthThe PR removes the lock when returning objects: Before: public IEnumerable<object> GetObjectsAtDepth(int depth)
{
if (!_objectsByDepth.TryGetValue(depth, out var objects))
{
return [];
}
lock (objects)
{
return objects.ToArray();
}
}After: public IReadOnlyCollection<object> GetObjectsAtDepth(int depth)
{
if (!_objectsByDepth.TryGetValue(depth, out var objects))
{
return [];
}
return objects; // HashSet<object> exposed directly
}Analysis: This change is probably safe IF the HashSets are never modified after ObjectGraph construction. Looking at ObjectGraphDiscoverer.cs:131 and :153, the ObjectGraph is created AFTER discovery completes, so concurrent modification shouldn't occur. However:
Recommendation: If performance is critical here, document clearly in the method/class that the returned collections MUST NOT be modified. Better yet, keep the defensive copy or switch to returning SuggestionsNone - fix the critical issues above first. VerdictThe struct conversion introduces design issues that outweigh the minimal performance benefits. The removal of unused methods/fields is good, but the architectural change needs reconsideration. |
|
Okay, looking closer I'm pretty sure everything but the logic used by I'll make a new PR when this is merged |
Remove unused methods and fields from
ObjectGraphIObjectGraphObjectGraphinto astructBefore
After