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

Add Path.TryGetTempPath() #17097

Closed
wants to merge 7 commits into from
Closed

Add Path.TryGetTempPath() #17097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
75c1b14
use existing api
MarcoRossignoli Mar 19, 2018
f99a46f
wip
MarcoRossignoli Mar 20, 2018
a024058
wip
MarcoRossignoli Mar 20, 2018
5ef6494
wip
MarcoRossignoli Mar 21, 2018
81bbed6
address PR feedback
MarcoRossignoli Mar 21, 2018
54d0ee2
non allocating string
MarcoRossignoli Mar 22, 2018
2b23a97
remove unuseful checked
MarcoRossignoli Mar 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions src/mscorlib/shared/System/IO/Path.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,35 @@ private static string RemoveLongPathPrefix(string path)
return path; // nop. There's nothing special about "long" paths on Unix.
}

private static void GetTempPath(ref ValueStringBuilder builder)
{
const string TempEnvVar = "TMPDIR";
const string DefaultTempPath = "/tmp/";

// Get the temp path from the TMPDIR environment variable.
int requiredSize = 0;
while ((requiredSize = Microsoft.Win32.Win32Native.GetEnvironmentVariable(TempEnvVar, builder.Span)) > builder.Capacity)
Copy link
Member Author

@MarcoRossignoli MarcoRossignoli Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@JeremyKuhne @Anipik to avoid allocation of string i need to use GetEnvironmentVariable(string lpName, Span<char> lpValue). I've exposed internal span and i'm pretty sure you don't want this. Can you help me with overload for GetEnvironmentVariable(string lpName, ref char lpTempFileName, int size) or GetEnvironmentVariable(string lpName, ValueStringBuilder builder) i don't find a way to cast ref char to char*

Copy link
Member

Choose a reason for hiding this comment

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

You need to use fixed to get the address (char*) of a ref char. If you follow the code down you'll see:

[DllImport(Interop.Libraries.Kernel32, CharSet = CharSet.Auto, SetLastError = true, BestFitMapping = false)]
private static extern unsafe int GetEnvironmentVariable(string lpName, char* lpValue, int size);

internal static unsafe int GetEnvironmentVariable(string lpName, Span<char> lpValue)
{
    fixed (char* lpValuePtr = &MemoryMarshal.GetReference(lpValue))
    {
        return GetEnvironmentVariable(lpName, lpValuePtr, lpValue.Length);
    }
}

What you should really do here is rewrite GetEnvironmentVariableCoreHelper to take a ValueStringBuilder. Take a look at the changes I made in dotnet/corefx@050bc33 for examples.

char* is equivalent to ref char in P\Invoke signatures. As such, the helper in Win32Native can just be removed and the Pinvoke can be changed to take a ref char as everything is calling it with a Span. You can avoid having to use the fixed statement that way. (Note that there is currently an issue in CoreRT that requires manually using fixed- if you look at P\Invokes in the shared folder you'll see #ifdefs that do it both ways.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok TYVM

Copy link
Member

Choose a reason for hiding this comment

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

You will have issues with making the same code build with CoreRT. The environment stuff is factored differently there. We may need a (public) no-allocation API to read the environment to make this work well. No-allocation environment access is likely a lot more useful than no-allocation temporary path generator.

Copy link
Member

Choose a reason for hiding this comment

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

You will have issues with making the same code build with CoreRT.

Ah yes, I forgot that while the P/Invoke isn't shared, this is. We can do the same hack on the P/Invoke there that I mentioned above no? (e.g. ifdef and manually pin the ref char)

We may need a (public) no-allocation API to read the environment to make this work well.

There isn't a need for it to be public to unblock this, is there? I agree that we should have a public API for it eventually.

Copy link
Member

@jkotas jkotas Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We should be using the same patterns internally as we are using for our public APIs. If the Try* pattern that returns bool and Span does not work for out internal use, it is unlikely going to work well for our users as well. This suggests that the pattern we are using for the public API is wrong.

This applies to both GetEnvironmentVariable or GetTempPath APIs.

I think we need to step back and do work on the API design. We may need to expose the ValueStringBuilder as public to make this work well.

Copy link
Member Author

Choose a reason for hiding this comment

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

@JeremyKuhne i go on with above idea(paying attention to corert) right? If i success with this could help also on new environment api.

Copy link
Member Author

@MarcoRossignoli MarcoRossignoli Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry @jkotas i've got a delay on refresh messages, i read your comment now. I wait your decision to understand how go on!I stay here and "listen". IMHO public ValueStringBuilder will be very useful.

Copy link
Member

Choose a reason for hiding this comment

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

@jkotas I'll open an issue on ValueStringBuilder and link to it. While I think we definitely need VSB type APIs, I'm not confident that not having the existing Try* and Span pattern becomes moot with said APIs. What if you want to write to fixed size span and don't want it grabbing from array pool? Does it make sense to have an option on ValueStringBuilder that doesn't allow it to grow? I'll discuss it in the issue.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/dotnet/corefx/issues/28379

{
// Reported size is greater than the buffer size. Increase the capacity.
builder.EnsureCapacity(requiredSize);
}

if (requiredSize == 0 && Runtime.InteropServices.Marshal.GetLastWin32Error() == Interop.Errors.ERROR_ENVVAR_NOT_FOUND)
{
builder.EnsureCapacity(DefaultTempPath.Length);
// If it's not set, just return the default path.
builder.Append(DefaultTempPath.AsSpan());
}
else
{
// If it is, return it, ensuring it ends with a slash.
if (!PathInternal.IsDirectorySeparator(builder[builder.Length - 1]))
{
builder.Append(PathInternal.DirectorySeparatorChar);
}
}
}

public static string GetTempPath()
{
const string TempEnvVar = "TMPDIR";
Expand Down
8 changes: 8 additions & 0 deletions src/mscorlib/shared/System/IO/Path.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,14 @@ public static bool TryJoin(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, R
return true;
}

public static bool TryGetTempPath(Span<char> destination, out int charsWritten)
{
charsWritten = 0;
ValueStringBuilder vsb = new ValueStringBuilder();
Path.GetTempPath(ref vsb);
return vsb.TryCopyTo(destination, out charsWritten);
}

private static string CombineInternal(string first, string second)
{
if (string.IsNullOrEmpty(first))
Expand Down
2 changes: 2 additions & 0 deletions src/mscorlib/shared/System/Text/ValueStringBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ public int Length

public int Capacity => _chars.Length;

public Span<char> Span => _chars;
Copy link
Member

Choose a reason for hiding this comment

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

You have AsSpan() that you should use

Copy link
Member Author

@MarcoRossignoli MarcoRossignoli Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I need a Span not a ReadOnlySpan to pass to api, right?or you mean create a custom modifiable Span and after Append?

Copy link
Member

Choose a reason for hiding this comment

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

If you want the ref char (which you do), you use GetPinnableReference().

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok TYVM


public void EnsureCapacity(int capacity)
{
if (capacity > _chars.Length)
Expand Down