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

Getting rid of "out" params in order to use ValueTuple / Nullability #12729

Open
dotNETSanta opened this issue May 21, 2019 · 5 comments
Open
Milestone

Comments

@dotNETSanta
Copy link

https://github.com/dotnet/coreclr/blob/9773db1e7b1acb3ec75c9cc0e36bd62dcbacd6d5/src/System.Private.CoreLib/shared/System/Buffers/Text/Utf8Parser/Utf8Parser.Integer.Signed.D.cs#L9-L81

Why cannot we repalce all those out params with for example ValueTuples or Nullability?

Current approach:

string s = (...);

if (int.TryParse(s, out var result)
{
	(...)
}

My suggestion:

A:

var result = int.TryParse(s);
if (result.Success)
{
	Console.WriteLine(result.Value);
}	

B:

var result = int.TryParse(s);
if (result.HasValue)
{
	Console.WriteLine(result);
}

A:

public (bool Success, int Value) TryParse(string s)
{
	if (...)
	{
	   return (false, default(int));
	}
	
	(...)
	
	return (true, result);
}

B:

public int? Value TryParse(string s)
{
	if (...)
	{
	   return null;
	}
	
	(...)
	
	return result;
}

I think it'd:

Made old code being more maintainable by updating old code to "modern standards"

We'd be able to get rid of side effects functions

How it'd affect performance?

@dotNETSanta dotNETSanta changed the title Getting rid of out params in order to use ValueTuple / Nullability Getting rid of "out" params in order to use ValueTuple / Nullability May 21, 2019
@gfoidl
Copy link
Member

gfoidl commented May 22, 2019

How it'd affect performance?

out-Arguments can be in registers, so it's fast. (Edit: inprecise, see correction from @mikedn below)
ValueTuple and nullable int are structs, so may end up in stack-space, which is memory and thus slower than registers. For the JIT it is also harder to optimize than simple register use, so code-gen for the out arguments may be better than using structs.

And your suggestion would be a huge breaking change.

@mikedn
Copy link
Contributor

mikedn commented May 22, 2019

Arguments can be in registers, so it's fast.

No, they are not. The out reference itself may be in a register but the actual value has to be in memory, you can't have a reference to a register.

ValueTuple and nullable int are structs, so may end up in stack-space, which is memory and thus slower than registers

That depends on the struct and the calling convention. A int? is actually returned in a register on both Win-x64 and Linux-x64. Though sadly the JIT doesn't generate good code in this case and goes through memory even in the end it does return the value a register.

For the JIT it is also harder to optimize than simple register use, so code-gen for the out arguments may be better than using structs.

No, it's the other way around. out parameters are a headache for JIT. Struct returns should be less of a problem (but they probably are, given the JIT's long term inability to handle structs properly).

Current approach: My suggestion:

I don't see what's good about your suggestion. The language was specifically improved to make out parameters more usable - you can declare the result variable inline. In your suggestion we basically go back to the old way of having to declare a variable before if. And extra code to reference struct's properties.

What you're suggesting requires an option type and language support for that. Without that, it's just a different and IMO worse way to do what you can already do today.

In any case, API suggestions belong in the corefx repository. Language suggestions belong in the csharplang repository.

@gfoidl
Copy link
Member

gfoidl commented May 22, 2019

The out reference

Oh, I simplified too much, so it got really inprecise and incorrect (which I didn't notice while writing). Thanks precising and correcting that!

out parameters are a headache for JIT

I definitely beliefe you, but out of interest:
My statement comes from experience that the JIT does a better job with inlining methods that have out arguments than methods that return structs (which are then "deconstructed" immediately at the caller, so such a method could be inlined too).
Further for structs the JIT zeros out memory (is this true for all structs?), whilst for out it just needs to take an adress.
This led me to the -- as you state wrong -- conclusion. Did I errournous infer this on behalf of JIT's problem with proper struct-handling?
Why is out so hard for the JIT? Because it needs to go to memory in the case of non-inlined callee?

@mikedn
Copy link
Contributor

mikedn commented May 22, 2019

My statement comes from experience that the JIT does a better job with inlining methods that have out arguments than methods that return structs (which are then "deconstructed" immediately at the caller, so such a method could be inlined too).

There definitely are issues with the way the JIT handle struct returns. It's enough to look at the simple case of int? that is returned in a register but the JIT assembles the 2 fields by going through memory:

       33C0                 xor      eax, eax
       8944242C             mov      dword ptr [rsp+2CH], eax
       C744242C2A000000     mov      dword ptr [rsp+2CH], 42
       C644242801           mov      byte  ptr [rsp+28H], 1
       488B442428           mov      rax, qword ptr [rsp+28H]

But issues like this are solvable.

Why is out so hard for the JIT? Because it needs to go to memory in the case of non-inlined callee?

Yes, it needs to go to memory and not only for the duration of the call - once passed via out/ref a local variable is marked "address exposed" and removed from pretty much all optimizations. Consider:

int sum = 0;
for (int i = 0; i < 100; i++)
    sum += i;
return sum;

generates

G_M55886_IG03:
       03C2                 add      eax, edx
       FFC2                 inc      edx
       83FA64               cmp      edx, 100
       7CF7                 jl       SHORT G_M55886_IG03

now

[MethodImpl(MethodImplOptions.NoInlining)]
static void log(ref int value) { }

int sum = 0;
log(ref sum);
for (int i = 0; i < 100; i++)
    sum += i;
return sum;

generates

G_M55886_IG03:
       8BD0                 mov      edx, eax
       03542420             add      edx, dword ptr [rsp+20H]
       89542420             mov      dword ptr [rsp+20H], edx
       FFC0                 inc      eax
       83F864               cmp      eax, 100
       7CEF                 jl       SHORT G_M55886_IG03

This issue is more difficult to solve:

  • Use interprocedural analysis to detect that log does not escape the address. Interprocedural analysis in a JIT? Well...
  • Make it so that the variable is only treated as address exposed in relevant regions of code. Problematic in the current JIT design.
  • Rely on CSE to CSE local variable loads in regions where it can be proved that the variable does not change despite being address exposed. That may work to an extent in the current JIT but the above example shows that it doesn't always help. Besides, doing it this way create more local variables which is another problem.

In contrast, structs that are not returned in register are returned via an hidden out parameter but that's special - the JIT could assume that the address won't be escaped. The callee will just store the return value at the supplied address and do nothing else with it.

That said, this should not give anyone the impression that struct returns are necessarily better. In fact I'd argue that they're worse in a certain way - consistency. The way a struct is returned depends on the struct and the calling convention. int? is returned in a register on win-x64 but long? is returned via memory. And float? is returned in an integer register and that requires extra work.

At least with out you always know what you're going to get. And the problem with the address exposed local variable can be avoided by developers by not passing "hot" local variables to ref/out parameters - pass a temporary copy instead. Not elegant but if you really need it you can do it.

@gfoidl
Copy link
Member

gfoidl commented May 22, 2019

@mikedn thank you! 👍

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 26, 2020
@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

6 participants