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

Provide API to reduce string allocations in user-defined custom converters #183

Closed
AArnott opened this issue Dec 21, 2024 · 5 comments · Fixed by #187
Closed

Provide API to reduce string allocations in user-defined custom converters #183

AArnott opened this issue Dec 21, 2024 · 5 comments · Fixed by #187
Labels
enhancement New feature or request

Comments

@AArnott
Copy link
Owner

AArnott commented Dec 21, 2024

See the CommonString struct defined in vs-streamjsonrpc (like this one) which currently uses MessagePack-CSharp APIs. We should expose equivalent APIs from this library, and/or expose a CommonString-like struct so that the user doesn't have to write it.

@AArnott AArnott added the enhancement New feature or request label Dec 21, 2024
@AArnott
Copy link
Owner Author

AArnott commented Dec 21, 2024

CC: @trippwill

@trippwill
Copy link

In adding support for this to vs-streamjsonrpc, we have a many instances of this pattern for CommonString like Version2, which would be awesome to simplify:

        if (!reader.TryReadStringSpan(out ReadOnlySpan<byte> valueBytes))
        {
            // TODO: More specific exception type
            throw new NBMP::MessagePackSerializationException(Resources.UnexpectedErrorProcessingJsonRpc);
        }

        // Recognize "2.0" since we expect it and can avoid decoding and allocating a new string for it.
        if (Version2.TryRead(valueBytes))
        {
            return Version2.Value;
        }

@AArnott
Copy link
Owner Author

AArnott commented Dec 21, 2024

Am I reading this right? It looks bad:

  if (!reader.TryReadStringSpan(out ReadOnlySpan<byte> valueBytes))
{
  // TODO: More specific exception type
  throw new NBMP::MessagePackSerializationException(Resources.UnexpectedErrorProcessingJsonRpc);
}

TryReadStringSpan is supposed to be able to return false when the string isn't contiguous in memory, so throwing in a perfectly legitimate (though uncommon) case seems broken.

@trippwill
Copy link

This is the code path from MessagePackFormatter. If we can't get the string as a span of bytes, there's nowhere to go:

        ReadOnlySpan<byte> valueBytes = MessagePack.Internal.CodeGenHelpers.ReadStringSpan(ref reader);
        if (Version2.TryRead(valueBytes))
        {
            return Version2.Value;
        }

@AArnott
Copy link
Owner Author

AArnott commented Dec 21, 2024

This is the code path from MessagePackFormatter

That one is fine, because if the string isn't contiguous, it'll allocate memory and make it contiguous. It's the if (!TryReadStringSpan) that's buggy.
But with the new API I'm developing in NB.MP, hopefully that code will go away anyway in favor of something simpler. So no worries about it for now.

@AArnott AArnott linked a pull request Dec 21, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants