Skip to content

Commit 65a3cb7

Browse files
Copilotrenemadsen
andcommitted
Optimize string manipulation in MySqlQueryStringFactory
- Replace string concatenation with StringBuilder for LEAST/GREATEST function evaluation - Reduces allocations from N×3 to 1 per PrepareCommand call - Processes replacements in single pass from start to end - Eliminates repeated string copying overhead - Updated PERFORMANCE_ANALYSIS.md to reflect fixes Co-authored-by: renemadsen <[email protected]>
1 parent 9ba3ef2 commit 65a3cb7

File tree

2 files changed

+67
-26
lines changed

2 files changed

+67
-26
lines changed

PERFORMANCE_ANALYSIS.md

Lines changed: 36 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -80,17 +80,36 @@ For a CommandText of 1000 characters with a function at position 500:
8080
- Per execution cost: ~1-2 KB allocation + CPU cycles for string operations
8181

8282
#### Recommendation
83-
**SHOULD FIX**: Use `StringBuilder` for string manipulation:
83+
**FIXED**: Changed to use `StringBuilder` for string manipulation. The implementation now:
84+
- Uses a single `StringBuilder` pre-allocated to CommandText length
85+
- Processes replacements in a single pass from start to end
86+
- Reduces allocations from N×3 to 1 (where N is the number of function replacements)
87+
- Eliminates repeated string copying overhead
88+
8489
```csharp
85-
var sb = new StringBuilder(command.CommandText.Length);
86-
sb.Append(command.CommandText, 0, func.Index);
87-
sb.Append(func.EvaluatedValue);
88-
sb.Append(command.CommandText, func.Index + func.FunctionCall.Length,
89-
command.CommandText.Length - func.Index - func.FunctionCall.Length);
90-
command.CommandText = sb.ToString();
90+
if (functionsToReplace.Count > 0)
91+
{
92+
var sb = new StringBuilder(command.CommandText.Length);
93+
var lastIndex = 0;
94+
95+
foreach (var func in functionsToReplace.OrderBy(f => f.Index))
96+
{
97+
sb.Append(command.CommandText, lastIndex, func.Index - lastIndex);
98+
sb.Append(func.EvaluatedValue);
99+
lastIndex = func.Index + func.FunctionCall.Length;
100+
// ... parameter cleanup
101+
}
102+
103+
if (lastIndex < command.CommandText.Length)
104+
{
105+
sb.Append(command.CommandText, lastIndex, command.CommandText.Length - lastIndex);
106+
}
107+
108+
command.CommandText = sb.ToString();
109+
}
91110
```
92111

93-
This would reduce allocations from N to 1 (where N is the number of function replacements).
112+
**Status**: ✅ FIXED in this commit
94113

95114
---
96115

@@ -332,11 +351,11 @@ This is a best practice for reflection-heavy code. The change caches expensive r
332351

333352
## Summary and Recommendations
334353

335-
### Immediate Actions Required
336-
1. **FIX CRITICAL**: MySqlJsonTableExpression infinite loop bug (line 100)
354+
### Completed Actions
355+
1.**FIXED CRITICAL**: MySqlJsonTableExpression infinite loop bug (line 100) - changed `i++` to `j++`
356+
2.**FIXED MODERATE**: MySqlQueryStringFactory string concatenation - now uses StringBuilder for efficient string manipulation
337357

338358
### High Priority
339-
2. **OPTIMIZE**: MySqlQueryStringFactory string concatenation (use StringBuilder)
340359
3. **OPTIMIZE**: MySqlParameterInliningExpressionVisitor single-pass evaluation
341360

342361
### Medium Priority
@@ -351,14 +370,15 @@ This is a best practice for reflection-heavy code. The change caches expensive r
351370

352371
## Conclusion
353372

354-
The .NET 10 migration introduced several performance concerns, with one critical bug that must be fixed immediately. The most significant performance issues relate to string manipulation and multiple-pass algorithms in hot code paths. However, some positive changes like reflection caching were also introduced.
373+
The .NET 10 migration introduced several performance concerns, with one critical bug and several moderate issues that have now been addressed. The most significant performance issues related to string manipulation and potential infinite loops have been fixed in this commit.
355374

356-
**Overall Risk Assessment**: MODERATE
357-
- 1 critical bug (infinite loop)
358-
- 2-3 moderate performance concerns (string allocation, multiple passes)
375+
**Overall Risk Assessment**: LOW (after fixes)
376+
- ~~1 critical bug (infinite loop)~~ ✅ FIXED
377+
- ~~1 moderate performance concern (string allocation)~~ ✅ FIXED
378+
- 1 moderate performance concern remaining (multi-pass evaluation in LEAST/GREATEST)
359379
- Several minor concerns that may add up under high load
360380

361-
**Recommendation**: Address the critical bug immediately, then profile real-world workloads to determine if the moderate issues cause measurable performance degradation before optimizing.
381+
**Recommendation**: The critical bug and primary performance issue have been resolved. Profile real-world workloads to determine if the remaining moderate issue (multi-pass LEAST/GREATEST evaluation) causes measurable performance degradation before further optimization.
362382

363383
---
364384

src/EFCore.MySql/Query/Internal/MySqlQueryStringFactory.cs

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -180,22 +180,43 @@ protected virtual void PrepareCommand(DbCommand command)
180180
.ToList();
181181

182182
// Replace LEAST/GREATEST function calls with their evaluated values
183-
foreach (var func in functionsToReplace)
183+
// Use StringBuilder to avoid multiple string allocations
184+
if (functionsToReplace.Count > 0)
184185
{
185-
command.CommandText = command.CommandText.Substring(0, func.Index) +
186-
func.EvaluatedValue +
187-
command.CommandText.Substring(func.Index + func.FunctionCall.Length);
186+
var sb = new StringBuilder(command.CommandText.Length);
187+
var lastIndex = 0;
188188

189-
// Remove the parameters used in this function from the parameters collection
190-
var paramMatches = _extractParameterRegex.Value.Matches(func.FunctionCall);
191-
foreach (Match match in paramMatches)
189+
// Process replacements from start to end (list is sorted descending, so reverse it)
190+
foreach (var func in functionsToReplace.OrderBy(f => f.Index))
192191
{
193-
var paramName = match.Value;
194-
if (command.Parameters.Contains(paramName))
192+
// Append text from last position to current function
193+
sb.Append(command.CommandText, lastIndex, func.Index - lastIndex);
194+
195+
// Append the evaluated value
196+
sb.Append(func.EvaluatedValue);
197+
198+
// Update last index to after the function call
199+
lastIndex = func.Index + func.FunctionCall.Length;
200+
201+
// Remove the parameters used in this function from the parameters collection
202+
var paramMatches = _extractParameterRegex.Value.Matches(func.FunctionCall);
203+
foreach (Match match in paramMatches)
195204
{
196-
command.Parameters.Remove(command.Parameters[paramName]);
205+
var paramName = match.Value;
206+
if (command.Parameters.Contains(paramName))
207+
{
208+
command.Parameters.Remove(command.Parameters[paramName]);
209+
}
197210
}
198211
}
212+
213+
// Append remaining text after last replacement
214+
if (lastIndex < command.CommandText.Length)
215+
{
216+
sb.Append(command.CommandText, lastIndex, command.CommandText.Length - lastIndex);
217+
}
218+
219+
command.CommandText = sb.ToString();
199220
}
200221

201222
var validParameters = (limitGroupsWithParameter

0 commit comments

Comments
 (0)