Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Make DangerousGetPinnableReference internal and remove DangerousTryGetArray #15557

Merged
merged 2 commits into from
Dec 24, 2017

Conversation

ahsonkhan
Copy link

@ahsonkhan ahsonkhan commented Dec 17, 2017

Fixes:
https://github.com/dotnet/corefx/issues/25412
https://github.com/dotnet/corefx/issues/25615

Also fixes https://github.com/dotnet/corefx/issues/23881

Blocked until all uses of DangerousGetPinnableReference and DangerousTryGetArray are replaced with MemoryMarshal APIs (in progress).

Related corefx PR: dotnet/corefx#25964

Following the staging plan from here: https://github.com/dotnet/corefx/issues/23881#issuecomment-343767740

  • Add MemoryExtensions.GetReference/TryGetArray
  • Convert all uses of DangerousGetPinnableReference/DangerousTryGetArray in coreclr, corefx, corert, corefxlab, aspnet, ... to MemoryExtensions.GetReference
  • Change DangerousGetPinnableReference to whatever we like to make it fit the pinning pattern and remove DangerousTryGetArray.

Doing it this way will avoid the need for complex staging or things being on the floor for extensive periods of time.

cc @jkotas, @stephentoub, @KrzysztofCwalina, @davidfowl

@ahsonkhan ahsonkhan added the blocked Issue/PR is blocked on something - see comments label Dec 17, 2017
@@ -128,7 +128,7 @@ internal ReadOnlySpan(ref T ptr, int length)
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
[EditorBrowsable(EditorBrowsableState.Never)]
public ref T DangerousGetPinnableReference()
internal ref readonly T DangerousGetPinnableReference()
{
return ref _pointer.Value;
Copy link
Member

Choose a reason for hiding this comment

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

We will also want to change this to return null for empty Spans, and maybe rename it to GetPinnableReference. It can wait for future change that makes it public again.

@ahsonkhan
Copy link
Author

@dotnet-bot test this please

@ahsonkhan ahsonkhan removed the blocked Issue/PR is blocked on something - see comments label Dec 21, 2017
@ahsonkhan
Copy link
Author

@dotnet-bot test this please

1 similar comment
@ahsonkhan
Copy link
Author

@dotnet-bot test this please

@ahsonkhan ahsonkhan merged commit 4d0e363 into dotnet:master Dec 24, 2017
@ahsonkhan ahsonkhan deleted the HideDangerousAPIs branch December 24, 2017 01:43
@michellemcdaniel
Copy link

@ahsonkhan This change seems to have broken our PGO count collection scenario in the dotnet/optimization repository on Linux. In this scenario, we build coreclr/clrjit with the pgoinstrument flag, and then copy coreclr, clrjit, and System.Private.Corelib.dll into a dotnet cli install that we have pulled down before running scenarios. After this change, when we try to do a dotnet restore, we get the following failure:

/mnt/resource/j/w/Private/dotnet_optimization/master/CLRx64LINmasPGO/output/CLRx64LINmas/PGO/cli/x64/sdk/2.2.0-preview1-007853/NuGet.targets(104,5): error : Method not found: '!0 ByRef System.ReadOnlySpan`1.DangerousGetPinnableReference()'.

Do you have any idea why we would be seeing this?

@ahsonkhan
Copy link
Author

@adiaaida, there is likely a version mismatch somewhere in the set of things that are being pulled down resulting in this method being called when it shouldn't. Unfortunately, I am OOF atm, and will be able to look into this issue until next week.

It is possible that I didn't replace all calls to (the now hidden) DangerousGetPinnableReference with MemoryMarshal.GetReference. Although, I couldn't find any such uses in the coreclr repo.

PGO count collection scenario in the dotnet/optimization repository on Linux

Which repo is this? If it is still using DangerousGetPinnableReference somewhere, that needs to be replaced.

@michellemcdaniel
Copy link

michellemcdaniel commented Jan 2, 2018

The repo itself doesn't actually use this. What we do is we run the cli's dotnet-install.sh script to pull down dotnet, build a private build of coreclr with pgo instrument then copy over the coreclr, clrjit and system.private.corelib bits into the dotnet install we pulled down using the script. We then run dotnet new console and dotnet restore, which is what fails with this error. I wonder if what we are pulling down from myget is stale in comparison to the system.private.corelib bits (like maybe we haven't been pushing new bits since you checked in this change), since without the copying of bits, we get:

/home/dotnet-bot/optimization/output/CLRx64LINmas/PGO/cli/test/test.csproj : error NU1102: Unable to find package Microsoft.NETCore.App with version (>= 2.1.0-preview1-26023-01)
/home/dotnet-bot/optimization/output/CLRx64LINmas/PGO/cli/test/test.csproj : error NU1102:   - Found 20 version(s) in nuget.org [ Nearest version: 2.0.4 ]
/home/dotnet-bot/optimization/output/CLRx64LINmas/PGO/cli/test/test.csproj : error NU1102:   - Found 1 version(s) in CliFallbackFolder [ Nearest version: 2.0.0-preview1-002111-00 ]
/home/dotnet-bot/optimization/output/CLRx64LINmas/PGO/cli/test/test.csproj : error NU1102:   - Found 0 version(s) in https://dotnet.myget.org/F/dotnet-buildtools/api/v3/index.json

@ahsonkhan
Copy link
Author

@adiaaida, can you still repro this issue?

@michellemcdaniel
Copy link

No. It looks like things worked themselves out.

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.

ReadOnlySpan<T>.DangerousGetPinnableReference should return a readonly reference
3 participants