Skip to content

Conversation

@kroymann
Copy link
Contributor

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

This addresses the issue outlined in #1997 where Image.LoadAsync(stream) would incorrectly use synchronous IO if the stream is seekable. The fix is simply to remove the optimization that avoids prebuffering the stream into an in-memory buffer if the stream is seekable, and instead always prebuffer the stream. This ensures that async IO is always used to read the stream.

kroymann and others added 5 commits February 14, 2022 16:01
@JimBobSquarePants
Copy link
Member

Everything looks good so far from an implementation perspective. We'll need to figure out a strategy now to compare the new Async performance with Main.

@JimBobSquarePants
Copy link
Member

@antonfirsov I just pushed a change to ChunkedMemoryStream that allows the size of the pooled buffer chunks to scale with the total allocation.

Allocation blocks match this scale.

LargeChunks = Min(4Mb, MemoryAllocator.BufferCapacityInBytes())
LargeChunkThreshold = Min(1Mb, LargeChunk / 4)
SmallChunks = Min(128Kb, LargeChunks / 32)

Once the total allocation within the stream hits our threshold we immediately switch to allocating larger chunk sizes.

I chose the the the values for the following reasons

  • Small @ 128Kb because that's the default chunk size RecycledMemoryStream uses.
  • Threshold @ 1Mb because that's the threshold of the ArrayPool. I was tempted to go for 512Kb here.
  • Large @ 4Mb because that matches the best performing pool size for our current allocator.

I'm not precious about any of these values to if you feel like they should change then please suggest alternatives.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm approving this but want a second opinion before merging.

@antonfirsov
Copy link
Member

antonfirsov commented Feb 26, 2022

I want to take a look next week. Quite distracted from everything programming-related thanks to the ongoing Fourth Reich experiment on my continent, right in the neighborhood.

@JimBobSquarePants
Copy link
Member

Of course mate. Absolute horror show over there!

@antonfirsov
Copy link
Member

antonfirsov commented Mar 13, 2022

@kroymann can you please give me permission to push to your branch? (Thought it is always allowed for contributors for PR branches.)
I want to push my load test changes:
antonfirsov@63e897e

As an alternative, feel free to cherry pick it.

This was referenced Jul 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Signifies a binary breaking change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants