Skip to content
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

Add PipeWriter.UnflushedBytes #48913

Closed
davidfowl opened this issue Mar 1, 2021 · 8 comments · Fixed by #54164
Closed

Add PipeWriter.UnflushedBytes #48913

davidfowl opened this issue Mar 1, 2021 · 8 comments · Fixed by #54164
Labels
api-approved API was approved in API review, it can be implemented area-System.IO.Pipelines help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Mar 1, 2021

Background and Motivation

Sometimes its useful to know how many bytes have been buffered and not flushed by the pipe writer. This can be used to optimize how much the other side can see at once (a scenario where the pipe writer wants to write with a minimal chunk size). The JSON serializer does a similar thing today when it writing to the output stream

Proposed API

namespace System.IO.Pipelines
{
    public abstract class PipeWriter 
    {
+       pubic virtual long? UnflushedBytes { get; }
    }
}

Usage Examples

async Task CopyWithChunks(Stream source, PipeWriter writer, long flushThreshold)
{
    while (true)
    {
        var memory = writer.GetMemory();
        var read = await stream.ReadAsync(memory);

        if (read == 0)
        {
            break;
        }

        writer.Advance(read);

        // Flush if we've reached the threshold
        if (writer.UnflushedBytes > flushThreshold)
        {
            await writer.FlushAsync();
        }
    }

    if (writer.UnflushedBytes > 0)
    {
        await writer.FlushAsync();
    }
}

Alternatives

namespace System.IO.Pipelines
{
    public abstract class PipeWriter 
    {
+       pubic virtual long? BytesWritten { get; }
    }
}

Risks

The nullable makes it a bit tricky to use but we need to indicate that a derived PipeWriter might not have implemented it.

@davidfowl davidfowl added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 1, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO.Pipelines untriaged New issue has not been triaged by the area owner labels Mar 1, 2021
@davidfowl davidfowl changed the title Add PipeWriter.UnFlushedBytes Add PipeWriter.UnflushedBytes Mar 1, 2021
@davidfowl davidfowl added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 1, 2021
@Wraith2
Copy link
Contributor

Wraith2 commented Mar 1, 2021

Are you sure it needs to be nullable?

@BrennanConroy BrennanConroy removed the untriaged New issue has not been triaged by the area owner label Mar 1, 2021
@BrennanConroy BrennanConroy added this to the 6.0.0 milestone Mar 1, 2021
@davidfowl
Copy link
Member Author

Are you sure it needs to be nullable?

I was thinking about it being 0 and what sort of defensive code you'd need to write if it was zero or some sentinal value. Maybe it should default to int.MaxValue 😄

@Wraith2
Copy link
Contributor

Wraith2 commented Mar 1, 2021

Given that the natural range will be 0>max I'd have thought the sentinel would be one of the usual suspects of -1 or MinValue. Being nullable just raises the question of what null means to me, negatives I can apply similar experience to and gloss over but null? not sure what that'd mean. For high perf code if it has no real meaning avoiding unneeded nullables seemed wise.

@davidfowl
Copy link
Member Author

So:

async Task CopyWithChunks(Stream source, PipeWriter writer, long flushThreshold)
{
    while (true)
    {
        var memory = writer.GetMemory();
        var read = await stream.ReadAsync(memory);

        if (read == 0)
        {
            break;
        }

        writer.Advance(read);

        // Flush if we've reached the threshold
        if (writer.UnflushedBytes == -1 || writer.UnflushedBytes > flushThreshold)
        {
            await writer.FlushAsync();
        }
    }

    if (writer.UnflushedBytes == -1 || writer.UnflushedBytes > 0)
    {
        await writer.FlushAsync();
    }
}

If you make it int.MaxValue the code could stay the same 😄 . I'm worried about subtle bugs when people use the field for arithmetic operations on the sentinel value.

@davidfowl
Copy link
Member Author

Forgot this is a dupe of #26818

@HurricanKai
Copy link
Member

HurricanKai commented May 25, 2021

Would be very helpful if this was added. I am currently faking this behaviour myself by tracking the write count myself.
Another reason why this is very useful is that there is only FlushAsync and not Flush, so when writing high-performance serialization code (not using async) there is no way to flush during that, I usually want to flush in the outer serialization loop, as that's async. There it's helpful to know the amount of written to the pipe.
Also helpful for diagnostics and debugging. (Also yes I'm late 😅)

@bartonjs
Copy link
Member

bartonjs commented May 25, 2021

Video

  • The discussion favored this being a Try method to better communicate to the caller (and code reviewer) that the implementation might not have an answer; then circled back and split the Nullable into a virtual Can property and a virtual throwing property.
namespace System.IO.Pipelines
{
    partial class PipeWriter 
    {
         public virtual bool CanGetUnflushedBytes => false;
         public virtual long UnflushedBytes => throw new NotImplementedException(some message here);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 25, 2021
@davidfowl davidfowl added the help wanted [up-for-grabs] Good issue for external contributors label May 26, 2021
@davidfowl
Copy link
Member Author

@Wraith2 do you want to take a stab at implementing this?

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 14, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.IO.Pipelines help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants