|
| 1 | +# DotTerritory Efficiency Analysis Report |
| 2 | + |
| 3 | +## Executive Summary |
| 4 | + |
| 5 | +This report documents performance optimization opportunities identified in the DotTerritory codebase. The analysis focused on finding allocation-heavy patterns, particularly related to LINQ operations and unnecessary intermediate collections. A total of 10 efficiency issues were identified across the codebase that could improve performance, especially for high-frequency operations in geospatial calculations. |
| 6 | + |
| 7 | +## Methodology |
| 8 | + |
| 9 | +The analysis used targeted searches for common inefficiency patterns: |
| 10 | +- `.ToArray()` and `.ToList()` materializations |
| 11 | +- `.Select()` and `.Reverse()` LINQ operations |
| 12 | +- String concatenations in hot paths |
| 13 | +- Multiple enumerations of collections |
| 14 | +- Unnecessary intermediate allocations |
| 15 | + |
| 16 | +## Identified Issues |
| 17 | + |
| 18 | +### 1. ⚠️ HIGH PRIORITY: Unnecessary `Reverse().ToArray()` in Territory.Along.cs |
| 19 | + |
| 20 | +**File:** `src/DotTerritory/Territory.Along.cs` |
| 21 | +**Line:** 11 |
| 22 | +**Severity:** High |
| 23 | +**Impact:** Used in hot path for coordinate walking operations |
| 24 | + |
| 25 | +**Current Code:** |
| 26 | +```csharp |
| 27 | +if (distance < Length.Zero) |
| 28 | +{ |
| 29 | + line = new LineString(line.Coordinates.Reverse().ToArray()); |
| 30 | + distance *= -1; |
| 31 | +} |
| 32 | +``` |
| 33 | + |
| 34 | +**Issue:** |
| 35 | +- `Reverse()` creates an intermediate `IEnumerable<Coordinate>` |
| 36 | +- `ToArray()` then materializes this into a new array |
| 37 | +- Two allocations instead of one |
| 38 | + |
| 39 | +**Recommended Fix:** |
| 40 | +```csharp |
| 41 | +if (distance < Length.Zero) |
| 42 | +{ |
| 43 | + var reversed = line.Coordinates.ToArray(); |
| 44 | + Array.Reverse(reversed); |
| 45 | + line = new LineString(reversed); |
| 46 | + distance *= -1; |
| 47 | +} |
| 48 | +``` |
| 49 | + |
| 50 | +**Benefits:** |
| 51 | +- Eliminates intermediate enumerable allocation |
| 52 | +- Uses in-place `Array.Reverse()` which is more efficient |
| 53 | +- Reduces GC pressure |
| 54 | + |
| 55 | +**Status:** ✅ FIXED in this PR |
| 56 | + |
| 57 | +--- |
| 58 | + |
| 59 | +### 2. String Concatenation for HashSet Keys in Territory.CleanCoords.cs |
| 60 | + |
| 61 | +**File:** `src/DotTerritory/Territory.CleanCoords.cs` |
| 62 | +**Line:** 68 |
| 63 | +**Severity:** Medium |
| 64 | +**Impact:** Called for each coordinate comparison during cleaning operations |
| 65 | + |
| 66 | +**Current Code:** |
| 67 | +```csharp |
| 68 | +var alreadySeenCoordinates = new HashSet<string>(); |
| 69 | +// ... |
| 70 | +var key = $"{point.X},{point.Y}"; |
| 71 | +if (!alreadySeenCoordinates.Add(key)) |
| 72 | + continue; |
| 73 | +``` |
| 74 | + |
| 75 | +**Issue:** |
| 76 | +- String allocation for each coordinate comparison |
| 77 | +- Unnecessary string formatting overhead |
| 78 | +- HashSet of strings has higher memory footprint than value types |
| 79 | + |
| 80 | +**Recommended Fix:** |
| 81 | +```csharp |
| 82 | +var alreadySeenCoordinates = new HashSet<(double X, double Y)>(); |
| 83 | +// ... |
| 84 | +var key = (point.X, point.Y); |
| 85 | +if (!alreadySeenCoordinates.Add(key)) |
| 86 | + continue; |
| 87 | +``` |
| 88 | + |
| 89 | +**Benefits:** |
| 90 | +- Zero string allocations |
| 91 | +- ValueTuple has better memory layout |
| 92 | +- More efficient equality comparison |
| 93 | + |
| 94 | +--- |
| 95 | + |
| 96 | +### 3. Multiple Collection Enumerations in Territory.Centroid.cs |
| 97 | + |
| 98 | +**File:** `src/DotTerritory/Territory.Centroid.cs` |
| 99 | +**Lines:** 108-113 |
| 100 | +**Severity:** Low |
| 101 | +**Impact:** Called when computing centroids of feature collections |
| 102 | + |
| 103 | +**Current Code:** |
| 104 | +```csharp |
| 105 | +var centroids = features |
| 106 | + .Select(feature => Centroid(feature.Geometry)) |
| 107 | + .ToList(); |
| 108 | + |
| 109 | +var sumX = centroids.Select(point => point.X).Sum(); |
| 110 | +var sumY = centroids.Select(point => point.Y).Sum(); |
| 111 | +``` |
| 112 | + |
| 113 | +**Issue:** |
| 114 | +- Materializes to list with `ToList()` |
| 115 | +- Then uses `Select()` again which creates new enumerables |
| 116 | +- Could be done in a single pass |
| 117 | + |
| 118 | +**Recommended Fix:** |
| 119 | +```csharp |
| 120 | +var centroids = features |
| 121 | + .Select(feature => Centroid(feature.Geometry)) |
| 122 | + .ToList(); |
| 123 | + |
| 124 | +double sumX = 0, sumY = 0; |
| 125 | +foreach (var point in centroids) |
| 126 | +{ |
| 127 | + sumX += point.X; |
| 128 | + sumY += point.Y; |
| 129 | +} |
| 130 | +``` |
| 131 | + |
| 132 | +**Benefits:** |
| 133 | +- Single enumeration of the list |
| 134 | +- No intermediate LINQ allocations |
| 135 | +- More cache-friendly access pattern |
| 136 | + |
| 137 | +--- |
| 138 | + |
| 139 | +### 4. Unnecessary LINQ in Territory.Explode.cs |
| 140 | + |
| 141 | +**File:** `src/DotTerritory/Territory.Explode.cs` |
| 142 | +**Line:** 25 |
| 143 | +**Severity:** Low |
| 144 | +**Impact:** Called when exploding geometries into points |
| 145 | + |
| 146 | +**Current Code:** |
| 147 | +```csharp |
| 148 | +var points = coordinates |
| 149 | + .Select(coord => factory.CreatePoint(coord)) |
| 150 | + .ToArray(); |
| 151 | +``` |
| 152 | + |
| 153 | +**Issue:** |
| 154 | +- LINQ `Select()` creates intermediate enumerable |
| 155 | +- Could use pre-allocated array or loop |
| 156 | + |
| 157 | +**Recommended Fix:** |
| 158 | +```csharp |
| 159 | +var coordArray = coordinates.ToArray(); |
| 160 | +var points = new Point[coordArray.Length]; |
| 161 | +for (int i = 0; i < coordArray.Length; i++) |
| 162 | +{ |
| 163 | + points[i] = factory.CreatePoint(coordArray[i]); |
| 164 | +} |
| 165 | +``` |
| 166 | + |
| 167 | +**Benefits:** |
| 168 | +- Single allocation for the array |
| 169 | +- No LINQ overhead |
| 170 | +- More predictable performance |
| 171 | + |
| 172 | +--- |
| 173 | + |
| 174 | +### 5. Unnecessary LINQ for Coordinate Cloning in Territory.BezierSpline.cs |
| 175 | + |
| 176 | +**File:** `src/DotTerritory/Territory.BezierSpline.cs` |
| 177 | +**Line:** 30 |
| 178 | +**Severity:** Low |
| 179 | +**Impact:** Called during bezier spline generation |
| 180 | + |
| 181 | +**Current Code:** |
| 182 | +```csharp |
| 183 | +var coords = line.Coordinates |
| 184 | + .Select(c => new Coordinate(c)) |
| 185 | + .ToList(); |
| 186 | +``` |
| 187 | + |
| 188 | +**Issue:** |
| 189 | +- LINQ creates intermediate enumerable just for copying |
| 190 | +- `ToList()` allocates list infrastructure |
| 191 | + |
| 192 | +**Recommended Fix:** |
| 193 | +```csharp |
| 194 | +var originalCoords = line.Coordinates; |
| 195 | +var coords = new List<Coordinate>(originalCoords.Length); |
| 196 | +for (int i = 0; i < originalCoords.Length; i++) |
| 197 | +{ |
| 198 | + coords.Add(new Coordinate(originalCoords[i])); |
| 199 | +} |
| 200 | +``` |
| 201 | + |
| 202 | +**Benefits:** |
| 203 | +- Pre-sized list avoids resizing |
| 204 | +- No LINQ allocation overhead |
| 205 | +- Clearer intent |
| 206 | + |
| 207 | +--- |
| 208 | + |
| 209 | +### 6. Multiple ToArray() Allocations in Territory.LineChunk.cs |
| 210 | + |
| 211 | +**File:** `src/DotTerritory/Territory.LineChunk.cs` |
| 212 | +**Lines:** 71, 91, 102, 105 |
| 213 | +**Severity:** Medium |
| 214 | +**Impact:** Called when chunking lines into segments |
| 215 | + |
| 216 | +**Current Code:** |
| 217 | +```csharp |
| 218 | +var chunk = currentChunkCoords.ToArray(); |
| 219 | +// ... multiple times in the method |
| 220 | +``` |
| 221 | + |
| 222 | +**Issue:** |
| 223 | +- Multiple `ToArray()` calls throughout the method |
| 224 | +- Each creates a new array allocation |
| 225 | +- List grows dynamically causing multiple resizes |
| 226 | + |
| 227 | +**Recommended Fix:** |
| 228 | +- Consider using `ArrayPool<Coordinate>` for temporary storage |
| 229 | +- Pre-calculate approximate chunk sizes to pre-allocate lists |
| 230 | +- Reuse coordinate arrays where possible |
| 231 | + |
| 232 | +**Benefits:** |
| 233 | +- Reduced allocations through pooling |
| 234 | +- Better memory reuse |
| 235 | +- Lower GC pressure for repeated operations |
| 236 | + |
| 237 | +--- |
| 238 | + |
| 239 | +### 7. ToArray Without Pre-allocation in Territory.LineSliceAlong.cs |
| 240 | + |
| 241 | +**File:** `src/DotTerritory/Territory.LineSliceAlong.cs` |
| 242 | +**Line:** 134 |
| 243 | +**Severity:** Low |
| 244 | +**Impact:** Called during line slicing operations |
| 245 | + |
| 246 | +**Current Code:** |
| 247 | +```csharp |
| 248 | +return new LineString(slicedCoords.ToArray()); |
| 249 | +``` |
| 250 | + |
| 251 | +**Issue:** |
| 252 | +- List grows dynamically during building |
| 253 | +- Final `ToArray()` creates yet another allocation |
| 254 | +- Could pre-calculate size in many cases |
| 255 | + |
| 256 | +**Recommended Fix:** |
| 257 | +- If possible, estimate final size and pre-allocate |
| 258 | +- Consider using `ArrayPool<Coordinate>` for temporary storage |
| 259 | +- Build directly into appropriately-sized array |
| 260 | + |
| 261 | +**Benefits:** |
| 262 | +- Fewer allocations |
| 263 | +- Better memory locality |
| 264 | +- Reduced list resize operations |
| 265 | + |
| 266 | +--- |
| 267 | + |
| 268 | +### 8. Duplicate LINQ Chains in TerritoryUtils.cs |
| 269 | + |
| 270 | +**File:** `src/DotTerritory/TerritoryUtils.cs` |
| 271 | +**Lines:** 17-18 |
| 272 | +**Severity:** Low |
| 273 | +**Impact:** Utility method, impact depends on usage frequency |
| 274 | + |
| 275 | +**Current Code:** |
| 276 | +```csharp |
| 277 | +var firstPoint = coords.First(); |
| 278 | +var lastPoint = coords.Last(); |
| 279 | +``` |
| 280 | + |
| 281 | +**Issue:** |
| 282 | +- `First()` and `Last()` may enumerate the sequence |
| 283 | +- For arrays/lists this is fine, but for `IEnumerable<T>` it's wasteful |
| 284 | +- Could be optimized with pattern matching |
| 285 | + |
| 286 | +**Recommended Fix:** |
| 287 | +```csharp |
| 288 | +var coordArray = coords as Coordinate[] ?? coords.ToArray(); |
| 289 | +var firstPoint = coordArray[0]; |
| 290 | +var lastPoint = coordArray[^1]; |
| 291 | +``` |
| 292 | + |
| 293 | +**Benefits:** |
| 294 | +- Single materialization if needed |
| 295 | +- Direct array access instead of LINQ |
| 296 | +- Index-based access is faster |
| 297 | + |
| 298 | +--- |
| 299 | + |
| 300 | +### 9. Select().ToArray() Pattern in Territory.LineIntersect.cs |
| 301 | + |
| 302 | +**File:** `src/DotTerritory/Territory.LineIntersect.cs` |
| 303 | +**Line:** 126 (approximate) |
| 304 | +**Severity:** Low |
| 305 | +**Impact:** Called during line intersection calculations |
| 306 | + |
| 307 | +**Issue:** |
| 308 | +- Common pattern of `Select().ToArray()` creating intermediate enumerables |
| 309 | +- Could use pre-allocated array with loop |
| 310 | + |
| 311 | +**Recommended Fix:** |
| 312 | +- Replace LINQ chains with explicit loops |
| 313 | +- Pre-allocate arrays when size is known |
| 314 | +- Use for loops instead of Select |
| 315 | + |
| 316 | +**Benefits:** |
| 317 | +- Eliminates LINQ overhead |
| 318 | +- More predictable performance |
| 319 | +- Better for hot paths |
| 320 | + |
| 321 | +--- |
| 322 | + |
| 323 | +### 10. Coordinate Enumeration Inefficiencies in Territory.FeatureCollection.cs |
| 324 | + |
| 325 | +**File:** `src/DotTerritory/Territory.FeatureCollection.cs` |
| 326 | +**Severity:** Low |
| 327 | +**Impact:** Depends on how frequently feature collections are processed |
| 328 | + |
| 329 | +**Issue:** |
| 330 | +- Multiple methods traverse coordinate sequences |
| 331 | +- Coordinates may be enumerated multiple times |
| 332 | +- Could benefit from caching when reused |
| 333 | + |
| 334 | +**Recommended Fix:** |
| 335 | +- Cache coordinate arrays when they'll be reused |
| 336 | +- Avoid repeated traversals |
| 337 | +- Consider lazy evaluation patterns |
| 338 | + |
| 339 | +**Benefits:** |
| 340 | +- Reduced traversals |
| 341 | +- Better cache utilization |
| 342 | +- Lower CPU usage |
| 343 | + |
| 344 | +--- |
| 345 | + |
| 346 | +## Prioritization and Next Steps |
| 347 | + |
| 348 | +### High Priority |
| 349 | +1. **Territory.Along.cs** - Fixed in this PR ✅ |
| 350 | + - Hot path operation |
| 351 | + - Simple fix with immediate benefit |
| 352 | + |
| 353 | +### Medium Priority |
| 354 | +2. **Territory.CleanCoords.cs** - String concatenation |
| 355 | + - Moderate impact |
| 356 | + - Straightforward value tuple solution |
| 357 | + |
| 358 | +3. **Territory.LineChunk.cs** - Multiple ToArray() calls |
| 359 | + - Could benefit from ArrayPool |
| 360 | + - More complex refactoring needed |
| 361 | + |
| 362 | +### Low Priority |
| 363 | +4-10. Other LINQ and allocation issues |
| 364 | + - Lower impact or less frequently called |
| 365 | + - Good candidates for incremental improvements |
| 366 | + |
| 367 | +## Performance Testing Recommendations |
| 368 | + |
| 369 | +To validate these optimizations: |
| 370 | +1. Create benchmark suite using BenchmarkDotNet |
| 371 | +2. Focus on hot path operations (WalkAlong, Distance, etc.) |
| 372 | +3. Measure allocation rates and GC pressure |
| 373 | +4. Profile real-world usage patterns |
| 374 | +5. Consider memory-constrained scenarios |
| 375 | + |
| 376 | +## Conclusion |
| 377 | + |
| 378 | +The DotTerritory codebase is well-structured and functional. These optimization opportunities represent incremental improvements that could yield significant benefits in high-throughput scenarios. The fixes range from simple (Reverse().ToArray()) to more complex (ArrayPool usage), allowing for gradual implementation based on priority and available resources. |
| 379 | + |
| 380 | +The most impactful change (Territory.Along.cs) has been implemented in this PR as a proof of concept. Future PRs can address the remaining issues incrementally, with appropriate benchmarking to validate the improvements. |
| 381 | + |
| 382 | +--- |
| 383 | + |
| 384 | +**Report Generated:** October 6, 2025 |
| 385 | +**Analyzed By:** Devin AI |
| 386 | +**Session:** https://app.devin.ai/sessions/078e83b75da94204a924a2d1a8ac826c |
0 commit comments