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

Proposal: Replace 'in' with 'ref' in Serialize method when using TBufferWriter #321

Open
haqoff opened this issue Aug 15, 2024 · 1 comment

Comments

@haqoff
Copy link

haqoff commented Aug 15, 2024

Summary
I propose changing the 'in' parameter to 'ref' in the Serialize method signature to allow more efficient use of custom IBufferWriter implementations when working with structures like ArrayPoolBufferWriter.

Background
In the current implementation of the Serialize method, the TBufferWriter parameter is passed using the 'in' keyword:

public static void Serialize<T, TBufferWriter>(
    in TBufferWriter bufferWriter,
    in T? value,
    MemoryPackSerializerOptions? options = null)
    where TBufferWriter : IBufferWriter<byte>
{
    // Serialization logic
}

When a structure is passed with 'in', it is copied when properties and methods are called. This means that any changes made within the method will not affect the original structure outside of the method.

Example Issue
Consider a structure like ArrayPoolBufferWriter, which aims to avoid unnecessary object allocation:

public struct ArrayPoolBufferWriter : IBufferWriter<byte>, IDisposable
{
    public int WrittenCount;
    public int Capacity;
    public int FreeCapacity;
    public ReadOnlyMemory<byte> WrittenMemory;
    public ReadOnlySpan<byte> WrittenSpan;
    public void Reset()
    public void Advance(int count)
    public Memory<byte> GetMemory(int sizeHint = 0)
    public Span<byte> GetSpan(int sizeHint = 0)
    public void Dispose()
}

When passed as 'in' to the Serialize method, the structure is copied (for example, when calling Advance), and the internal state of the original structure remains unchanged, leading to incorrect results.

@neuecc
Copy link
Member

neuecc commented Sep 10, 2024

Thank you, that's right, it's strange that this is in (why didn't I make it ref...?)
I want to change it to ref, but there are some compatibility issues, so I'd like to think about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants