WIP - Fix performance and memory allocation issues in ReadPdhMultiString. - #5845
WIP - Fix performance and memory allocation issues in ReadPdhMultiString.#5845jcarino wants to merge 1 commit into
Conversation
|
|
|
jcarino We can not merge the contribution until you sign CLA. |
|
Ah, |
|
CLA has now been signed. |
| char nextChar = (char)next4; | ||
| if (nextChar == '\0') | ||
| { | ||
| strColl.Add(sb.ToString()); |
There was a problem hiding this comment.
Adding to strColl should occur outside the while loop. The updated logic causes the contents of sb to be dropped on the floor by the break statement when next4 == 0.
Essentially, replace the original allSubstringsWithNulls += (char)next4 statement with sb.Append(nextChar) and move the strColl.Add statement to the original location after the while loop.
There was a problem hiding this comment.
You're right, but adding to strColl only outside the while loop has some memory implications (has to alloc at least one extra string before Split). Can we just add another strColl.Add(sb.ToString()); outside the while to catch whatever is left in sb?
|
Dan Travison (@dantraMSFT) Please clarify - this file is excluded from compilation - is it acceptable to change it without tests? And will we use it later? |
Ilya (@iSazonov) Thanks for bringing it up :) You are right that we don't have a way to officially validate the changes. Travis Plunk (@TravisEz13), any thoughts? For background: the perf counter cmdlets are disabled due to unsupported APIs. But the files are left in place so that
|
|
We should not make changes that are not tested. I think we can postpone this change until the API is ported in the future. I've seen that CoreFX team still doesn't have a consensus how to do it. Also if we decide to use Windows Compatibility Pack and it will support this API, then we could also include this in the compilation. |
|
Not only is it not tested. The file is not being compiled. I don't think we can take any changes to this file except possibly adding a comment to this effect and pointing to the issue to add the CmdLets back using supported APIs. #4306 |
|
I think we should close the PR until we compile this code again. |
|
jcarino The consensus is that we cannot take changes to this code at this time. |
PR Summary
String concats in ReadPdhMultiString have been replaced with a StringBuilder to significantly reduce heap allocation and GC.
ReadPdhMultiString reads characters one by one from a pointer location into a string, then splits that string into a string collection (on the null character '\0'). However, it does this through a looped string concat, allocating a new string on the heap for each character read in. This has significant GC implications for large strings. The string concats have been replaced with a string builder, and the strings are split and added to the StringCollection on the fly.
PR Checklist
Note: Please mark anything not applicable to this PR
NA.WIP:to the beginning of the title and remove the prefix when the PR is ready.