Skip to content

Use BinaryPrimitives for ObjectDataBuilder primitive emit logic #102394

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.

Sign up for GitHub

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

Jump to bottom
Merged
jkotas merged 9 commits into from
May 18, 2024
Merged

Use BinaryPrimitives for ObjectDataBuilder primitive emit logic #102394

jkotas merged 9 commits into from
May 18, 2024

Conversation

@PaulusParssinen
Copy link
Contributor

@PaulusParssinen PaulusParssinen commented May 17, 2024 •
edited
Loading

To allow more efficient use of the shared ArrayBuilder<T>, I added some Span<T> overloads under #if NET preprocessor.

These new ArrayBuilder<T> overloads will allow future improvements in other projects that utilize it.

@PaulusParssinen
Add ArrayBuilder.Append(ROS<T>)
9fe722c 
- Only for .NET because of NS2.0 projects use it too: Microsoft.NET.HostModel and ILVerification
@PaulusParssinen
Use BinaryPrimitives to emit integer primitives in ObjectDataBuilder
f7e82e6
@PaulusParssinen
Use BinaryPrimitives to emit reserved integer primitives in ObjectDat…
2caf45b 
…aBuilder

* Add ArrayBuilder.AsSpan overload to make this possible
@PaulusParssinen
Mark some ArrayBuilder members as readonly
94cb4cc
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 17, 2024
@PaulusParssinen
Copy link
Contributor Author

PaulusParssinen commented May 17, 2024 •
edited
Loading

Benchmark (Repo)

BenchmarkDotNet v0.13.12, Windows 10 (10.0.19045.4170/22H2/2022Update)
AMD Ryzen 7 3700X, 1 CPU, 16 logical and 8 physical cores
.NET SDK 9.0.100-preview.3.24204.13
  [Host]     : .NET 9.0.0 (9.0.24.17209), X64 RyuJIT AVX2
  DefaultJob : .NET 9.0.0 (9.0.24.17209), X64 RyuJIT AVX2
Details
Method Version N Mean Error Code Size Allocated
EmitUShort Main 10 39.99 ns 0.187 ns 699 B 192 B
EmitUShort Stackalloc 10 70.61 ns 0.868 ns 239 B 216 B
EmitUShort GetSpan 10 41.98 ns 0.870 ns 611 B 216 B
EmitUInt Main 10 82.10 ns 1.307 ns 1,300 B 280 B
EmitUInt Stackalloc 10 71.00 ns 0.641 ns 232 B 280 B
EmitUInt GetSpan 10 41.27 ns 0.319 ns 609 B 280 B
EmitLong Main 10 191.08 ns 0.858 ns 1,246 B 432 B
EmitLong Stackalloc 10 73.54 ns 0.171 ns 236 B 400 B
EmitLong GetSpan 10 46.35 ns 0.143 ns 631 B 400 B
EmitUShort Main 100 208.62 ns 1.712 ns 708 B 712 B
EmitUShort Stackalloc 100 397.58 ns 1.392 ns 239 B 760 B
EmitUShort GetSpan 100 154.35 ns 1.106 ns 632 B 760 B
EmitUInt Main 100 473.43 ns 1.415 ns 1,298 B 1248 B
EmitUInt Stackalloc 100 405.54 ns 1.070 ns 232 B 1272 B
EmitUInt GetSpan 100 176.13 ns 0.810 ns 633 B 1272 B
EmitLong Main 100 1,621.39 ns 3.852 ns 1,259 B 2296 B
EmitLong Stackalloc 100 448.50 ns 8.776 ns 236 B 2288 B
EmitLong GetSpan 100 200.78 ns 0.264 ns 614 B 2288 B
EmitUShort Main 1000 1,188.01 ns 3.851 ns 698 B 4368 B
EmitUShort Stackalloc 1000 3,576.12 ns 16.749 ns 239 B 4440 B
EmitUShort GetSpan 1000 1,110.59 ns 3.153 ns 621 B 4440 B
EmitUInt Main 1000 4,337.01 ns 12.272 ns 1,260 B 8488 B
EmitUInt Stackalloc 1000 3,450.58 ns 6.285 ns 232 B 8536 B
EmitUInt GetSpan 1000 1,164.84 ns 5.453 ns 623 B 8536 B
EmitLong Main 1000 15,190.00 ns 109.599 ns 1,247 B 16704 B
EmitLong Stackalloc 1000 3,786.44 ns 9.474 ns 242 B 16720 B
EmitLong GetSpan 1000 1,508.27 ns 29.400 ns 623 B 16720 B

jkotas
jkotas reviewed May 18, 2024
View reviewed changes
@PaulusParssinen
Follow ValueStringBuilder's AppendSpan implementation
bd0acd9
@PaulusParssinen
Don't inline ArrayBuilder resize
1508892 
* This follows ValueStringBuilder and other buffer builder types.
@PaulusParssinen
Initialize ArrayBuilder._items with empty array
5b48ae2 
* Removes the null-checks

* ToArray() already would have return empty array. And this isn't used in TypeLoader as far as I know.
@PaulusParssinen
Revert "Initialize ArrayBuilder._items with empty array"
2ff11b0 
This reverts commit 5b48ae2.

* Forgot that all places use default(ArrayBuilder<T>) to initialize, zeroizing the fields
jkotas
jkotas approved these changes May 18, 2024
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

@jkotas jkotas merged commit 5474ab5 into dotnet:main May 18, 2024
@PaulusParssinen PaulusParssinen deleted the opt-object-data-emit branch May 18, 2024 17:36
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
@PaulusParssinen @Ruihan-Yin
Use BinaryPrimitives for ObjectDataBuilder primitive emit logic (do…
ba4f845 
…tnet#102394)

* Add ArrayBuilder.Append(ROS<T>)

- Only for .NET because of NS2.0 projects use it too: Microsoft.NET.HostModel and ILVerification

* Use BinaryPrimitives to emit integer primitives in ObjectDataBuilder

* Use BinaryPrimitives to emit reserved integer primitives in ObjectDataBuilder

* Add ArrayBuilder.AsSpan overload to make this possible

* Mark some ArrayBuilder members as readonly

* Ask ArrayBuilder for a buffer instead of stackalloc + append

* Follow ValueStringBuilder's AppendSpan implementation

* Don't inline ArrayBuilder resize

* This follows ValueStringBuilder and other buffer builder types.
@github-actions github-actions bot locked and limited conversation to collaborators Jun 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotas jkotas jkotas approved these changes

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

+1 more reviewer

@rhuijben rhuijben rhuijben left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-crossgen2-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@PaulusParssinen @rhuijben @jkotas