-
-
Notifications
You must be signed in to change notification settings - Fork 887
Memory allocation allocation tracker test helper #2082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
77bb287
3872c60
af7d84a
1557bae
2a1ae5c
a30c468
399d71d
d9af239
99d1266
558ff99
cd7c91c
f597507
f4f521c
03d0b63
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,7 +65,8 @@ protected override void Decompress(BufferedReadStream stream, int byteCount, int | |
| jpegDecoder.ParseStream(stream, scanDecoderGray, CancellationToken.None); | ||
|
|
||
| // TODO: Should we pass through the CancellationToken from the tiff decoder? | ||
| CopyImageBytesToBuffer(buffer, spectralConverterGray.GetPixelBuffer(CancellationToken.None)); | ||
| using var decompressedBuffer = spectralConverterGray.GetPixelBuffer(CancellationToken.None); | ||
| CopyImageBytesToBuffer(buffer, decompressedBuffer); | ||
|
Comment on lines
+68
to
+69
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we also have separate PR-s for the Jpeg+Tiff+WebP+Bmp fixes or just for PNG?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all the decoder facing fixes are actually in here...but we will need a set of follow up PRs to get full coverage. |
||
| break; | ||
| } | ||
|
|
||
|
|
@@ -81,7 +82,8 @@ protected override void Decompress(BufferedReadStream stream, int byteCount, int | |
| jpegDecoder.ParseStream(stream, scanDecoder, CancellationToken.None); | ||
|
|
||
| // TODO: Should we pass through the CancellationToken from the tiff decoder? | ||
| CopyImageBytesToBuffer(buffer, spectralConverter.GetPixelBuffer(CancellationToken.None)); | ||
| using var decompressedBuffer = spectralConverter.GetPixelBuffer(CancellationToken.None); | ||
| CopyImageBytesToBuffer(buffer, decompressedBuffer); | ||
| break; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TotalUndisposedAllocationCountwas intended to report global allocation count. If I get the docs ofAsyncLocal<T>right, the PR changes the semantics, so it becomes specific to an asynchronous execution flow, and will reset outside of the flow. I don't think this public behavior change is beneficial for the users ofMemoryDiagnostics.TotalUndisposedAllocationCount. We need a separate API forMemoryAllocatorValidator.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It way its setup it will only report the local context when its been overwritten in tests (internal api) by default the backing fields are backs by the
MemoryDiagnostics.Defaultstatic instance which will be used in production usage outside of testing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now, I missed the purpose of the branching. Though probably still better to keep the internal API as minimalistic and non-intrusive as possible.