Corefxlab: WritableBuffer / OutputWriter / IOutput: Combine the Ensure() method and Span property; possibly rename to `GetSpan`

[mgravell]

You will always be calling Ensure() before fetching .Span, and any time you call Ensure() you must re-fetch the .Span, because it could now be different. As such: having the property is redundant. The "I want to enlarge it, but I'm not going to want the span" scenario seems absurdly unlikely to the point of incredulity.

Instead of:

writer.Ensure(); // "give me something"
var span = writer.Span;

or

writer.Ensure(8); // "give me at least 8"
var span = writer.Span;

It could more usefully just be used as:

var span = writer.Ensure(); // "give me something"

or

var span = writer.Ensure(8); // "give me at least 8"

This will simplify a hot path and reduce a lot of extra code.

Additionally, it feels like Span<byte> GetSpan(int minLength = 1) might be a better API signature:

• "min" because it is the minimum required, not the actual amount requested
• "length" because spans have .Length, not .Count

[pakrym]

Thought about this exactly today. The only strange things is that we still want to keep Span in case you just want to fill available free space.

/cc @davidfowl @KrzysztofCwalina

[benaadams]

The only strange things is that we still want to keep Span in case you just want to fill available free space.

Doesn't

var span = writer.Ensure();

allow you to do this? Can you provide more detail; its it so you can detect when there is no more space? e.g. 0-length span?

[mgravell]

Additional: this would also avoid an unnecessary span fetch; currently the span is fetched when you create the writer, in the OutputWriter constructor. In virtually all cases, the first thing the consumer is going to do is : call Ensure - because "I know it has some space" is a rare event. So that could be removed.

Additional additional:

Ensure could avoid a call down here; since Advance already decreases the span (_span = _span.Slice(count);), it should be entirely possible to short-circuit if we already have the space, to avoid a: going to the base, and b: re-fetching the span; suggestion (assuming all the changes proposed here):

    [MethodImpl(MethodImplOptions.NoInlining)]
    public Span<byte> GetSpan(int minLength = 1)
    {
        var span = _span;
        if (span.Length < minLength)
        {
            span = _span = _output.GetSpan(minLength);
        }
        return span;
    }

    public OutputWriter(T output)
    {
        _output = output;
        _span = default;
    }

Now of course the really controversial question: should the OutputWriter actually not keep slicing when you Advance, and just increment an Index that the consumer is expected to check - like with BufferReader... that is probably a separate issue, though :)

[KrzysztofCwalina]

As long as Ensure() returns the already available buffer, I think the change (Including change on IOutput) is good.

Also, we need to decide how a caller would say "the current buffer is too small, so enlarge it, but I don't care about the size of the enlarged buffer as long as it's larger than the current one". This is accomplished by parameterless Enlarge today.

[mgravell]

(replacing 3 comments with this one; and then I accidentally deleted this one when tidying up, oops!)

@KrzysztofCwalina that isn't what Ensure() (no explicit parameter) does; all it does is ensure that a: there's a block, and b: the size is at least big enough for what you've requested; it does not apply a "as long as it's larger than the current one" condition. Code:

private static void TestEnsure(WritableBuffer buffer)
{
    Console.WriteLine($"WritableBuffer initial: {buffer.Buffer.Length}");
    buffer.Ensure();
    Console.WriteLine($"after WritableBuffer.Ensure: {buffer.Buffer.Length}");
    buffer.Ensure();
    Console.WriteLine($"after another WritableBuffer.Ensure: {buffer.Buffer.Length}");
    var writer = OutputWriter.Create(buffer);
    Console.WriteLine($"OutputWriter initial: {writer.Span.Length}");
    writer.Ensure();
    Console.WriteLine($"after OutputWriter.Ensure: {writer.Span.Length}");
    writer.Ensure();
    Console.WriteLine($"after another OutputWriter.Ensure: {writer.Span.Length}");
}

example output:

WritableBuffer initial: 0
after WritableBuffer.Ensure: 4032
after another WritableBuffer.Ensure: 4032
OutputWriter initial: 4032
after OutputWriter.Ensure: 4032
after another OutputWriter.Ensure: 4032

only the very first Ensure() (when there isn't a block) actually does anything.

[KrzysztofCwalina]

b: the size is at least big enough for what you've requested

And what is "requested" when no augment is passed to Ensure?

BTW, this touches on something that I think we still need to do: we have lots of APIs with Ensure/Resize methods. We need to unify them and describe the contract of the unified method.

[mgravell]

@KrzysztofCwalina well, I can tell you what it currently means, but I suspect you mean "what should it mean?" - frankly my vote there would be: change the default parameter value to 1 and have it mean "just something non-empty", or even just remove the default completely so people at least ask themselves "what is the minimum I can accept here?" rather than just assuming "hey, I've got memory". In my view, making a number explicit - even if people pass 1 - would be good for reducing usage errors.

[mgravell]

Which makes me realise; actually the buffer doesn't need to retain the span
at all if it isn't using the same "span and offset" API from the reader.
All it needs to know is the size - an integer: set it when fetching (in
GetSpan if not big enough), and decrement it in advance. Retaining it
actually provides nothing. And then it doesn't even need to be a "ref
struct"! Although best to leave that there so it is open to implementation
changes.

On 11 Jan 2018 8:43 am, "Julius R Friedman" [e-mail]
wrote:

Something seems wrong.. Maybe because it's 3 a.m. ... but all I can see is
([sub r1, r2] [xor r1, r2]) cmp r1, r2; jge nOcopy; copy: sub r2, r1; mov
byte ptr r1, r2; sub r3, 1; cmp r3, 0; jle copy; nOcopy: ret;

....

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/dotnet/corefxlab/issues/2040#issuecomment-356864757,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABDsOVZUdcquFG6bd-r--jyXwleNX2pks5tJcmigaJpZM4RXw-8
.