Honor Concord's TruncatedString flag to reduce memory usage #68969
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
Area-Interactive
untriaged
ghost
added
the
Community
tmat
reviewed
Jul 11, 2023
src/ExpressionEvaluator/Core/Source/ResultProvider/Formatter.Values.cs
Outdated
Show resolved
Hide resolved
tmat
reviewed
Jul 11, 2023
...ExpressionEvaluator/Core/Source/ResultProvider/Helpers/DkmEvaluationResultFlagsExtensions.cs
Outdated
Show resolved
Hide resolved
Same suggestion from the Concord code review: I think we need to set a flag in the inspection context to indicate we want large strings rather than changing Refers to: src/ExpressionEvaluator/Core/Source/ResultProvider/Formatter.Values.cs:216 in 238d264. [](commit_id = 238d264, deletion_comment = False) |
| @@ -253,7 +253,8 @@ private static DkmEvaluationResultFlags GetFlags(DkmClrValue value, DkmInspectio | |||
| } | |||
| } | |||
|
|
|||
| if (!value.IsError() && value.HasUnderlyingString(inspectionContext)) | |||
| // check for truncated string before HasUndleryingString so we don't pull the string into memory just to set the flag | |||
| if (!value.IsError() && ((value.EvalFlags & (DkmEvaluationResultFlags)DkmEvaluationResultFlagsExtensions.TruncatedStringFlag) != 0 || value.HasUnderlyingString(inspectionContext))) | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Wouldn't HasUnderlyingString also check for TruncatedStringFlag when allocating the string?
(...does GetUnderlyingStringImpl call into GetValueString?)
| @@ -253,7 +253,8 @@ private static DkmEvaluationResultFlags GetFlags(DkmClrValue value, DkmInspectio | |||
| } | |||
| } | |||
|
|
|||
| if (!value.IsError() && value.HasUnderlyingString(inspectionContext)) | |||
| // check for truncated string before HasUndleryingString so we don't pull the string into memory just to set the flag | |||
There was a problem hiding this comment.
nit:
Suggested change
| // check for truncated string before HasUndleryingString so we don't pull the string into memory just to set the flag | |
| // check for truncated string before HasUnderlyingString so we don't pull the string into memory just to set the flag |
chuckries
force-pushed
the
dev/chuckr/LargeStrings
branch
from
238d264
to
6569c2e
Compare
| @@ -253,7 +253,8 @@ private static DkmEvaluationResultFlags GetFlags(DkmClrValue value, DkmInspectio | |||
| } | |||
| } | |||
|
|
|||
| if (!value.IsError() && value.HasUnderlyingString(inspectionContext)) | |||
| // check for truncated string before HasUndleryingString so we don't pull the string into memory just to set the flag | |||
| if (!value.IsError() && (value.EvalFlags.HasFlag(DkmEvaluationResultFlags.TruncatedString) || value.HasUnderlyingString(inspectionContext))) | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
isadorasophia
approved these changes
Jul 21, 2023
gregg-miskelly
approved these changes
Jul 22, 2023
|
@tmat I believe this is ready to merge, although CI is very unhappy. Did I do something to upset CI or is this a known issue? The errors don't look like anything I've touched. |
chuckries
force-pushed
the
dev/chuckr/LargeStrings
branch
3 times, most recently
from
166bcc8
to
94fd48e
Compare
…orks for large strings
chuckries
force-pushed
the
dev/chuckr/LargeStrings
branch
from
94fd48e
to
6e41739
Compare
tmat
approved these changes
Jul 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Concord is going to begin limiting the amount of memory it pulls from the target process for strings. Any string result that has been truncated is marked with a new flag, such that APIs like GetUnderlyingString can still work for things like string visualizers in order to surface the full bytes of the string.
This adds concords new flag as a temporary constant and ensures that HasUnderlyingValue and GetUnderlyingValue both still work without pulling in the full string memory unnecessarily.