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

Obsolete RuntimeHelpers.OffsetToStringData #35722

Merged
1 commit merged into from
Jun 9, 2020

Conversation

GrabYourPitchforks
Copy link
Member

Resolves #31406.

RuntimeHelpers.OffsetToStringData is the last remaining public API which exposes the concept of string's raw data as being inline directly after the object header. We want code to use string.GetPinnableReference instead. It's more future-proof and plays better with the experiments we're running on the string class.

/cc @jkoritzinsky, who assisted offline with the marshalling codegen. Thanks! :)
/cc @terrajobst to see if there are any concerns with the deprecation message. We obsoleted string.Copy in .NET Core 3.0 using a standard (no diagnostic id) obsolete attribute. I feel this is along the same lines so doesn't deserve its own diagnostic id.

char[] expectedValues = new char[] { 'a', 'b', 'c', 'd', 'e', 'f' };
string s = "abcdef";

fixed (char* values = s) // Compiler will use OffsetToStringData with fixed statements
Copy link
Member Author

Choose a reason for hiding this comment

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

The fixed keyword doesn't call OffsetToStringData any more, so this test wasn't actually testing the right thing. I'm deleting this test since we already have coverage over in StringTests.cs:

// Finally, ensure that GetPinnableReference matches the legacy 'fixed' keyword.
DynamicMethod dynamicMethod = new DynamicMethod("tester", typeof(bool), new[] { typeof(string) });
ILGenerator ilGen = dynamicMethod.GetILGenerator();
LocalBuilder pinnedLocal = ilGen.DeclareLocal(typeof(object), pinned: true);
ilGen.Emit(OpCodes.Ldarg_0); // load 'input' and pin it
ilGen.Emit(OpCodes.Stloc, pinnedLocal);
ilGen.Emit(OpCodes.Ldloc, pinnedLocal); // get the address of field 0 from pinned 'input'
ilGen.Emit(OpCodes.Conv_I);
ilGen.Emit(OpCodes.Call, typeof(RuntimeHelpers).GetProperty("OffsetToStringData").GetMethod); // get pointer to start of string data
ilGen.Emit(OpCodes.Add);
ilGen.Emit(OpCodes.Ldarg_0); // get value of input.GetPinnableReference()
ilGen.Emit(OpCodes.Callvirt, typeof(string).GetMethod("GetPinnableReference"));
// At this point, the top of the evaluation stack is traditional (fixed char* = input) and input.GetPinnableReference().
// Compare for equality and return.
ilGen.Emit(OpCodes.Ceq);
ilGen.Emit(OpCodes.Ret);
Assert.True((bool)dynamicMethod.Invoke(null, new[] { input }));

@GrabYourPitchforks
Copy link
Member Author

I looked through the mono code base and couldn't find any calls to the OffsetToStringData property getter. There are implementations of this method, but obviously we need to keep those around. Somebody may want to double-check my work.

GetKnownMethod("GetPinnableReference", null)));
codeStream.EmitStLoc(vPinnedCharRef);
codeStream.EmitLdLoc(vPinnedCharRef);
codeStream.Emit(ILOpcode.conv_u);
Copy link
Member

Choose a reason for hiding this comment

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

Would there be a downside to moving this conv_u to below lCommonExit? Then the duplicate one in the other branch could be removed.

Copy link
Member

@stephentoub stephentoub May 1, 2020

Choose a reason for hiding this comment

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

I guess maybe it's better for the same type to be on the stack prior to the branching?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the stack should be the same at all possible entry points to the branch. In one case the stack will have an i4, and in the other case the stack will have a char&. I was concerned that this would break things.

The codegen pattern here is identical to the codegen pattern Roslyn emits for pattern fixed statements. See Sharplab. Honestly I think some minor optimizations are possible, but I was nervous about straying from what the compiler itself already implements.

@@ -9093,6 +9093,7 @@ public static partial class RuntimeFeature
}
public static partial class RuntimeHelpers
{
[System.ObsoleteAttribute("OffsetToStringData is obsolete. Use string.GetPinnableReference() instead.")]
Copy link
Member

Choose a reason for hiding this comment

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

Should we be following @terrajobst's new plan for obsoletion?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a good question. :)

@GrabYourPitchforks
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ghost
Copy link

ghost commented Jun 9, 2020

Hello @GrabYourPitchforks!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit f929bed into dotnet:master Jun 9, 2020
@davidfowl
Copy link
Member

Did we obsolete something in the BCL??!? 👍🏿👍🏿👍🏿👍🏿🔥🔥🔥🔥🔥👍🏿

@GrabYourPitchforks
Copy link
Member Author

@davidfowl I know, right!? 😃

@mjsabby
Copy link
Contributor

mjsabby commented Jun 9, 2020

Do you already know about System.Text.Encodings.Web.dll?

@GrabYourPitchforks
Copy link
Member Author

@mjsabby not sure the context - was that comment meant for a different issue?

@mjsabby
Copy link
Contributor

mjsabby commented Jun 9, 2020

Sorry for the lack of context!
System.Text.Encodings.Web.dll has a member reference to get_OffsetToStringData. It's the only one in an .NET Core 3.1 application (including asp.net core)

@GrabYourPitchforks
Copy link
Member Author

Because in the 3.1 release it's still compiled against an older target framework (netstandard2.1). The latest nightly packages contain a build that's compiled against netcoreapp proper. So that DLL will contain the expected calls to string.GetPinnableReference instead of the now-obsolete API.

@GrabYourPitchforks GrabYourPitchforks deleted the obs_strdata branch June 9, 2020 18:24
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API proposal: Obsolete RuntimeHelpers.OffsetToStringData
4 participants