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

Use ArrayPool in CSTRMarshaler.ConvertToNative #113991

Closed
wants to merge 1 commit into from

Conversation

HJLeee
Copy link
Contributor

@HJLeee HJLeee commented Mar 27, 2025

This reduces temporary byte array creation during p/invoke string param marshaling.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 27, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 27, 2025
@@ -103,13 +104,19 @@ internal static unsafe IntPtr ConvertToNative(int flags, string strManaged, IntP
// wasting memory on systems with multibyte character sets where the buffer we end up with is often much
// smaller than the upper bound for the given managed string.

byte[] bytes = AnsiCharMarshaler.DoAnsiConversion(strManaged,
fBestFit: 0 != (flags & 0xFF), fThrowOnUnmappableChar: 0 != (flags >> 8), out nb);
byte[] bytes = ArrayPool<byte>.Shared.Rent(checked((strManaged.Length + 1) * Marshal.SystemMaxDBCSCharSize));
Copy link
Member

Choose a reason for hiding this comment

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

All of the marshallers in this file are quite legacy. There are many other opportunities to update.
I'd suggest to not touch it.

@jkotas
Copy link
Member

jkotas commented Mar 28, 2025

Looks like this change is triggering a JIT bug:

https://helix.dot.net/api/2019-06-17/jobs/0e6c868f-9ce9-4ad2-8e79-f61acd524e57/workitems/System.Security.Cryptography.OpenSsl.Tests/console
https://helix.dot.net/api/2019-06-17/jobs/0e6c868f-9ce9-4ad2-8e79-f61acd524e57/workitems/System.Runtime.InteropServices.Tests/console

Assert failure(PID 34542 [0x000086ee], Thread: 498545 [0x79b71]): Assertion failed '((typeHistogram != nullptr) || (methodHistogram != nullptr)) && "Expected at least one handle histogram when inserting probes"' in 'System.StubHelpers.CSTRMarshaler:ConvertToNative(int,System.String,long):long' during 'Profile instrumentation' (IL size 263; hash 0xc31c0dcd; Instrumented Tier0)

    File: /Users/runner/work/1/s/src/coreclr/jit/fgprofile.cpp:2084

cc @dotnet/jit-contrib

@jkotas
Copy link
Member

jkotas commented Mar 28, 2025

This reduces temporary byte array creation during p/invoke string param marshaling.

What's the signature you have encountered it with? Have you considered switching to LibraryImport to fix the problem? How does LibraryImport perf compare to what you get with this fix for the signature you have hit the problem with?

@jkotas jkotas added area-Interop-coreclr area-System.Runtime.InteropServices tenet-performance Performance related issue and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners area-System.Runtime.InteropServices labels Mar 28, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

@HJLeee
Copy link
Contributor Author

HJLeee commented Mar 28, 2025

What's the signature you have encountered it with? Have you considered switching to LibraryImport to fix the problem? How does LibraryImport perf compare to what you get with this fix for the signature you have hit the problem with?

Thank you for LibraryImport tip.
Tizen applications and custom libraries still use DllImport directly as they should be built on .NET 6 base env. for compatibility in production.

We are trying to reduce GC count/pause during UI drawing and the temporary byte arrays during p/invoke are the top most garbage creation in System.Byte[] type.
(Let me put screenshot of nettrace after I clear internal firewall.)
I thought this could be easily reduced thanks to ArrayPool.

@huoyaoyuan
Copy link
Member

Tizen applications and custom libraries still use DllImport directly as they should be built on .NET 6 base env.

Updating main branch would not improve .NET 6 environment. Such performance change typically doesn't meet the bar for backport. Moreover, 6.0 has reached EOL. If you provide your build, you can do update in your branch.

@HJLeee
Copy link
Contributor Author

HJLeee commented Mar 28, 2025

We'll use .NET 10 next year (hopefully)

@AaronRobinsonMSFT
Copy link
Member

We'll use .NET 10 next year (hopefully)

At that point then LibraryImport can be used. I do not think it is appropriate to start changing these marshallers without clear and convincing data that it will substantially improve them. Additionally, if string marshalling is an issue, then I would consider doing manual marshalling where full control can be achieved. That is possible with DllImport already.

@agocke
Copy link
Member

agocke commented Mar 29, 2025

Closing, as we are unlikely to take any changes here.

@agocke agocke closed this Mar 29, 2025
@HJLeee
Copy link
Contributor Author

HJLeee commented Mar 30, 2025

Thank you for the time and consideration.

@HJLeee HJLeee deleted the use_arraypool branch March 30, 2025 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Interop-coreclr community-contribution Indicates that the PR has been added by a community member tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants