Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Faster multi-element serialization for primitive types #1872

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

Merged

Conversation

pCYSl5EDgo
Copy link
Contributor

Rewrite of #1744

@pCYSl5EDgo pCYSl5EDgo changed the title Faster multi-element handling Faster multi-element serialization for primitive types Jul 4, 2024
Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Most of the hardware accelerated code is beyond me. But I have a couple questions at least.

src/MessagePack/Internal/UnsafeRefSerializeHelper.cs Outdated Show resolved Hide resolved
src/MessagePack/Internal/UnsafeRefSerializeHelper.cs Outdated Show resolved Hide resolved
@AArnott AArnott added this to the v3.0 milestone Jul 16, 2024
Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

The product changes look good. How are we looking for test coverage? In particular, given the 16-element boundary that is interesting, it would seem prudent to have tests that cover lists of various sizes including at and around that 16-length boundary. Do we already have such tests, and are they running on .NET 8 such that they'll cover your new code?

@pCYSl5EDgo
Copy link
Contributor Author

No. There aren't, I think.
I will add those tests.

@AArnott
Copy link
Collaborator

AArnott commented Jul 17, 2024

There is an awful amount of noise in the failure logs, but there does appear to be a legitimate failure here.

@pCYSl5EDgo
Copy link
Contributor Author

pCYSl5EDgo commented Jul 18, 2024

I've fixed bugs.

I have a question about bool[]? Deserialize(ref MessagePackReader reader, MessagePackSerializerOptions options).
Where should I place the simd-accelerated deserialization helper code?
Append it to UnsafeRefSerializeHelper.cs or write another file such as UnsafeRefDeserializeHelper.cs?
The simd-accelerated deserialization helper code does not share any code with the serialization helper methods.

@AArnott
Copy link
Collaborator

AArnott commented Jul 18, 2024

Where should I place the simd-accelerated deserialization helper code?

Given the length of the existing file, my vote is create a second file.

@AArnott
Copy link
Collaborator

AArnott commented Jul 18, 2024

Do you want me to merge this as-is, or do you want to add the deserialization optimization to it first?

@pCYSl5EDgo
Copy link
Contributor Author

pCYSl5EDgo commented Jul 18, 2024

Ok.
I'll create another PR of bool[] deserialization with a second file.
Please merge this PR as-is.

@AArnott AArnott merged commit a6c3e21 into MessagePack-CSharp:develop Jul 18, 2024
4 of 5 checks passed
@pCYSl5EDgo pCYSl5EDgo deleted the faster-multi-element-handling branch July 18, 2024 15:29
@pCYSl5EDgo pCYSl5EDgo mentioned this pull request Jul 19, 2024
14 tasks
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

Successfully merging this pull request may close these issues.

2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.