Add Path.TryGetTempPath() #17097

MarcoRossignoli · 2018-03-21T16:54:04Z

Closes coreclr part of https://github.com/dotnet/corefx/issues/28255
There is a test ready to push here , tested on Windows and Ubuntu.
cc: @JeremyKuhne @Anipik

MarcoRossignoli · 2018-03-21T17:04:00Z

src/mscorlib/shared/System/IO/Path.cs

+        {                        
+            charsWritten = 0;
+
+            var tmpPath = Path.GetTempPath();


i want to avoid this allocation if destination < tmpPath.Length, on Windows i could use ValueStringBuilder but on unix side we don't have

private static void GetTempPath(ref ValueStringBuilder builder)

on Path.Unix.cs there is

public static string GetTempPath()

with Environment.GetEnvironmentVariable(TempEnvVar) string allocation.
is there private static string GetEnvironmentVariableCoreHelper(string variable, Span<char> buffer) for unix?

is there private static string GetEnvironmentVariableCoreHelper(string variable, Span buffer) for unix?

As part of having this we should write the lower level pieces we need. :) I'd make an internal one that takes a ValueStringBuilder. I'm looking into what it would take to polish up that class and make it public in the future, btw.

@JeremyKuhne @Anipik i get the point, can you help me to find where Environment.GetEnvironmentVariable() pinvoke for unix are defined?If i follow code with VS i reach Win32Native only.

In this case the actual P/Invoke is provided by the PAL. So the same code is used in both places. The cross-plat model the native parts of the runtime uses is that it has Unix implementations for all of the Win32 APIs. It simply includes the PAL when building for Unix instead of the Windows headers/libs. In the managed code this abstraction is also leveraged. Anything in Win32Native has a Unix implementation. The source file that is included when building for Unix is coreclr\src\mscorlib\src\Interop\Unix\Interop.Libraries.cs.

As such, you can use this straight-up:

coreclr/src/mscorlib/src/Microsoft/Win32/Win32Native.cs

Line 365 in 57ee22e

internal static unsafe int GetEnvironmentVariable(string lpName, Span<char> lpValue)

Anipik · 2018-03-21T17:24:09Z

src/mscorlib/shared/System/IO/Path.cs

+        public static bool TryGetTempPath(Span<char> destination, out int charsWritten)
+        {                        
+            charsWritten = 0;
+


nit: extraLine

Anipik · 2018-03-21T17:24:12Z

src/mscorlib/shared/System/IO/Path.cs

+                return false;            
+
+            tmpPath.AsSpan().CopyTo(destination);
+


nit: extraLine

@Anipik is there a guideline for "extraLine"?

Anipik · 2018-03-21T17:24:24Z

src/mscorlib/shared/System/IO/Path.cs

+            tmpPath.AsSpan().CopyTo(destination);
+
+            charsWritten = tmpPath.Length;
+


nit: extraline

Anipik · 2018-03-21T17:28:23Z

src/mscorlib/shared/System/IO/Path.cs

+        {                        
+            charsWritten = 0;
+
+            var tmpPath = Path.GetTempPath();


use string instead var keyword. we use var if we can know the type of the object from RHS

jkotas · 2018-03-21T20:13:14Z

@JeremyKuhne Do we have examples of where the no-allocation version of this API makes a difference?

You are about to create a temporary file. Creating a temporary file is orders of magnitude more expensive operation than just allocating a string.

I think the no-allocation version of these APIs are adding negative value to the platform.

jkotas · 2018-03-21T20:15:22Z

We are stabilizing for .NET Core 2.1. New API additions need to wait once we start working for next version.

JeremyKuhne · 2018-03-21T20:18:58Z

Do we have examples of where the no-allocation version of this API makes a difference?

Yes, anywhere you need to build paths based off of the temporary folder location. While it isn't common in all applications, it is very common in some (build systems, designers, etc.).

jkotas · 2018-03-21T20:22:25Z

If somebody calls this on a hot path, they should just call the method once and cache the value. It is going to be much more efficient for them than just them calling this API again and again.

JeremyKuhne · 2018-03-21T20:32:35Z

If somebody calls this on a hot path, they should just call the method once and cache the value.

Sure, for those that have the ability to do so. While it is unlikely, this value can change. If you want to be strictly correct you shouldn't cache it. That is why I didn't cache the existing overload- although I considered it. :)

MarcoRossignoli · 2018-03-22T15:23:33Z

src/mscorlib/shared/System/IO/Path.Unix.cs

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


@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*

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.)

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.

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.

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.

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

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.

@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.

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

JeremyKuhne · 2018-03-22T17:50:57Z

src/mscorlib/shared/System/Text/ValueStringBuilder.cs

@@ -34,6 +34,8 @@ public int Length

        public int Capacity => _chars.Length;

+        public Span<char> Span => _chars;


You have AsSpan() that you should use

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

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

MarcoRossignoli · 2018-04-07T20:05:33Z

Does using this remove the need for having a bool TryGet*(Span) overload? (see https://github.com/dotnet/coreclr/pull/17097/files#r176560435)

@JeremyKuhne @jkotas should i go on with this PR or Path.TryGetRandomFileName() and Path.TryGetTempPath() will be removed?(I noticed @Anipik work on Path.TryGetFullPath() and Path.TryGetRelativePath())

JeremyKuhne · 2018-04-09T17:58:36Z

should i go on with this PR or Path.TryGetRandomFileName() and Path.TryGetTempPath() will be removed?(I noticed @Anipik work on Path.TryGetFullPath() and Path.TryGetRelativePath())

TryGetRandomFileName() won't change based on the ValueStringBuilder discussions/investigations, so that you can go ahead with if you like. I'll update the issues in the next day or so to reflect where we're at on the other discussions.

MarcoRossignoli · 2018-04-09T18:36:55Z

@JeremyKuhne ok i'll work on that next days while waiting!Feel free to assig to me if you like! https://github.com/dotnet/corefx/issues/28256

MarcoRossignoli · 2018-05-15T16:16:20Z

@JeremyKuhne any news on this?

JeremyKuhne · 2018-05-16T00:24:13Z

@MarcoRossignoli Sorry, with the 2.1 release, 3.0 disclosure/planning, and Build I haven't been able to get a lot of traction yet. Thank you so much for your patience. I need to spend some time chatting with @terrajobst about what we do here and perhaps revisit in API review again. It isn't clear that we can or should use BufferReader/Writer for these. ValueStringBuilder is a much lighter-weight solution, but, on the flip side, it is still another class.

MarcoRossignoli · 2018-05-16T07:04:19Z

@MarcoRossignoli Sorry...

no problem @JeremyKuhne i didn't want to put more pressure, this is only a light ping 😄!For now i'm working on other PR!Thank you very much for you fast reply!

JeremyKuhne · 2018-06-19T18:09:04Z

Reviewing these APIs again. This one in particular we might drop as the end user can potentially cache. It isn't strictly correct to cache, however, as the temp directory can be changed at any time (but would be colossally stupid to do so).

JeremyKuhne · 2018-06-19T18:09:32Z

Thanks for your efforts here, @MarcoRossignoli. :)

MarcoRossignoli added 4 commits March 19, 2018 12:54

use existing api

75c1b14

wip

f99a46f

wip

a024058

wip

5ef6494

MarcoRossignoli commented Mar 21, 2018

View reviewed changes

Anipik reviewed Mar 21, 2018

View reviewed changes

src/mscorlib/shared/System/IO/Path.cs Outdated

tmpPath.AsSpan().CopyTo(destination);

charsWritten = tmpPath.Length;

Copy link

Anipik Mar 21, 2018

Choose a reason for hiding this comment

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

nit: extraline

Anipik reviewed Mar 21, 2018

View reviewed changes

address PR feedback

81bbed6

MarcoRossignoli changed the title ~~WIP: Add Path.TryGetTempPath()~~ Add Path.TryGetTempPath() Mar 21, 2018

jkotas added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 21, 2018

JeremyKuhne added this to the Future milestone Mar 21, 2018

non allocating string

54d0ee2

MarcoRossignoli commented Mar 22, 2018

View reviewed changes

remove unuseful checked

2b23a97

JeremyKuhne reviewed Mar 22, 2018

View reviewed changes

RussKeldorph added the area-System.IO label Jun 8, 2018

JeremyKuhne closed this Jun 19, 2018

MarcoRossignoli deleted the 27418_path branch June 21, 2018 07:08

		tmpPath.AsSpan().CopyTo(destination);

		charsWritten = tmpPath.Length;

		@@ -34,6 +34,8 @@ public int Length

		public int Capacity => _chars.Length;

		public Span<char> Span => _chars;

Add Path.TryGetTempPath() #17097

Add Path.TryGetTempPath() #17097

Conversation

MarcoRossignoli commented Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Mar 21, 2018

jkotas commented Mar 21, 2018

JeremyKuhne commented Mar 21, 2018

jkotas commented Mar 21, 2018

JeremyKuhne commented Mar 21, 2018

MarcoRossignoli Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli commented Apr 7, 2018

JeremyKuhne commented Apr 9, 2018

MarcoRossignoli commented Apr 9, 2018 • edited Loading

MarcoRossignoli commented May 15, 2018

JeremyKuhne commented May 16, 2018

MarcoRossignoli commented May 16, 2018 • edited Loading

JeremyKuhne commented Jun 19, 2018

JeremyKuhne commented Jun 19, 2018

MarcoRossignoli commented Mar 21, 2018 •

edited

Loading

MarcoRossignoli Mar 21, 2018 •

edited

Loading

MarcoRossignoli Mar 22, 2018 •

edited

Loading

jkotas Mar 22, 2018 •

edited

Loading

MarcoRossignoli Mar 22, 2018 •

edited

Loading

MarcoRossignoli Mar 22, 2018 •

edited

Loading

MarcoRossignoli commented Apr 9, 2018 •

edited

Loading

MarcoRossignoli commented May 16, 2018 •

edited

Loading