Need a mechanism to break up long methods without call overhead

[CodeSetting]

Often it is helpful to break up long functions into several shorter functions (for readability). However, this can have significant performance consequences, especially in inner-loops.

I had originally posted, hoping that local functions might be that mechanism. Specifically, I hoped that under limited circumstances I could count on them being inlined. However, based on a reply from @jaredpar, I see that this may have been a simplistic hope.

I'm not certain what the correct fix is for this somewhat common problem. In my own specific case, if I were able to tell JIT to always inline a specific local function, it would meet my needs...perhaps with something more consistently honored than MethodImplOptions.AgressiveInlining.

A decoration on some sort of named local block, perhaps telling a pre-processor to cut/paste that section of code to a different location (where it was referenced) would also work.

Since the solutions sort of straddle multiple spaces: C# language, JIT, and C# compiler. I'll leave this post here for now.

Perhaps, I'll try posting elsewhere as well. More likely, I'll just learn to live with my long messy method :)

I was simply hoping I could get something like the following:

public class MyClass
{
  public void MyLongMethod()
  {
    Step1(); // Called in this single place and nowhere else
    Step2(); // Called in this single place and nowhere else
    Step3(); // Called in this single place and nowhere else
    MultipleUse(); // Called at least twice (first call)
    MultipleUse(); // Called at least twice (second call)

    void Step1()
    {
      // Logic for step 1
    }

    void Step2()
    {
      // Logic for step 2
    }

    void Step3()
    {
      // Logic for step 3
    }

    void MultipleUse
    {
      // Logic for multiple use
    }
  }
}

To build with the same performance as:

  public void MyLongMethod()
  {
    // Logic for step 1
    // Logic for step 2
    // Logic for step 3
    MultipleUse(); // Called at least twice (first call)
    MultipleUse(); // Called at least twice (second call)

    void MultipleUse
    {
      // Logic for multiple use
    }
  }
}

[CyrusNajmabadi]

It would be nice if I could be confident that a local function, with a single reference, would always be inlined. This would help to allay any performance concerns.

Inlining a function will not necessarily improve performance. In particular, it can stop other optimizations from happening. The best place for this to happen is in the JIT where they ahve all the information and can do things like PGO.

[CodeSetting]

As mentioned, I can understand that where a function has multiple sites of invocation, there are all sorts of considerations where the cost of inlining outweighs the benefits.

Can you provide a concrete example where inlining a single-reference, local function, thus saving a call, would not provide a performance improvement?

If this is the case, this suggests there are advantages beyond readability/maintainability to breaking up longer functions. However, this would be contrary to almost everything I've ever read on the topic or seen in my own benchmarking.

UPDATE: Also, as mentioned, I'm not against this happening in JIT as long as it was well-documented and consistent. As it is, I have a real-world issue where I have a necessarily very long method that is called inside an inner loop. I hate that I can't break it up for readability without trashing performance. I've gotten as far as I can using aggressive inlining hints, but the size limitations for JIT inlining are killing me on this one.

[CyrusNajmabadi]

Can you provide a concrete example where inlining a single-reference, local function, thus saving a call, would not provide a performance improvement?

Yes. This is the entire basis of many helpers like our argument throwing helpers. By inlining the contents of them, the outer method loses a lot of optimizations. It's very intentional, and a core part of how things are optimized that functions not be inlined automatically, but only when the runtime determines that it is actually a win (and very much not a loss) to do so.

[CodeSetting]

This is not a relevant example. The throw helpers you mentioned are usually called in multiple places. Otherwise, why bother writing them?Also, they are generally not local functions...sometimes private instance, but not local/nested.

[CyrusNajmabadi]

Even if they were called in a single location, it woudl be wrong to inline them.

[CodeSetting]

Thank you for your interest. Without a relevant example, I can't really comment on your opinion.

I've updated my original post to provide greater clarity, and an example, since the term "local function" might be confused for "private instance function" and since the meaning of single-reference might have been unclear.

[CyrusNajmabadi]

with technical arguments given in each as to why the other domain is the better place to fix it

Can you point me to the arguments given to why the C# compiler is the right place to do this? That seems... suspect, and we'd def want to know where the disconnect is coming from. As pointed out, it's fairly critical that we do not do this, lest we tank expectations and necessary code patterns that very much exist and must not change.

[CodeSetting]

I wish I could. I've been reading posts on this and related issues for over a decade.

The suggested fixes include a wide range of ideas: new language features, general JIT improvements, new JIT hints (attributes), and others. I've seen lots of technical arguments for why each is a bad idea and/or should be done elsewhere (most often JIT).

In fairness, some of them were truly terrible ideas :) What I've never seen, in all that time, is the slightest movement towards a solution.

I've definitely seen folks pointed to other places than runtime. Unfortunately, I can't trace it back to a specific post, after all this time, without a lot of work. Sorry.

[CyrusNajmabadi]

What I've never seen, in all that time, is the slightest movement towards a solution.

I've seen many solutions. The jit continues to get better. Driven by real world use cases they can use to adjust heuristics and policies :-)

[CodeSetting]

JIT is generally amazing. I honestly stand in awe of the optimizations it is able to predict. And, I've certainly seen many improvements in JIT's implementation over the years.

However, no optimization can ever be perfect. In this case, it probably "fails" out of cautious concern for many of the same issues you and Jared rightly outlined. It simply doesn't have enough information to make the "correct" decision.

JIT is great with aggressive inlining (and normal inlining) of small functions potentially consumed anywhere. This has been true for a very long time.

JIT absolutely sucks for a large function used in only a few places. It needs a non-existent hint (attribute) to let it know it is appropriate. This has also been true for a very long time. My mistake, in my earlier post, was believing I might have found a situation where this hint was not necessary.

This shortcoming in JIT is the topic of the posts I mentioned. The absence of movement in JIT, on this issue, has prompted other solutions, including mine, to be (somewhat desperately) suggested.

I stick by my assertion that I haven't seen movement in JIT (or elsewhere) towards a solution to this issue. If you know of one, please point me to it.

If there is an open forum somewhere specifically dedicated to discussing JIT, please let me know. I'll be happy to jump on it and champion a fix there. Thus far, I have not found one.

The closest I've seen are more general discussions in runtime, often about how aggressive (or not) AggressiveInlining is/should be. I wouldn't be adding anything new there that hasn't already been said a hundred times.

[CyrusNajmabadi]

there is an open forum somewhere specifically dedicated to discussing JIT, please let me know. I'll be happy to jump on it and champion a fix there. Thus far, I have not found one.

Dotnet/runtime.

I would create a repro for them and let them know about the problem.

[jaredpar]

However, for the limited case of a single-reference, local function, I see little risk.

I disagree with the little risk part of this assessment. @CyrusNajmabadi has outlined many of the risks but the ones that always come to mind for me are:

Inlining can reduce optimizations.

The JIT operates on a series of heuristics and inlining methods can trigger heuristics that cause the JIT to generate less efficient code. A classic example of that problem is throw. Historically the JIT does not optimize methods that use throw as good as it does methods that don't. In high perf code bases it's a common pattern to use throw helper methods to alleviate this problem

void M(string arg) {
  if (arg is null) {
    ThrowInvalidArg();
    return;
  }
  ... 

  void ThrowArgNull() => throw new InvalidArgumentException(); 
}

This is a case where inlining a method subverts the intent of the developer and generates less efficient code as a result.

The compiler cannot know what heuristics are in play at compile time because they come from the underlying runtime. A given piece of compiled code can execute on many different runtimes. As such we can't even limit such inlinings to places where it won't trip up JIT heuristics (except in a few narrow cases).

Inlining can introduce crashes

One reason for having local functions is to reduce the size of the current stack frame. A common pattern in recursive methods that except deep recursion is to use local functions for intermediate calculations. Essentially it's a scratch buffer to hold locals that don't need to persist across recursion calls. This reduces the total stack frame size and hence increases the depth to which recursion can happen.

void M(MyType input) {
  if (input ...) { 
    M(Reduce(input));
  }
}

  MyType Reduce(MyType input) { 
    Locals ... 
  }
}

Inlining here would once again subvert the intent of the developer, cause stack frame size to increase and potentially lead to program crashes.

This is a very real problem the C# compiler itself faces.

Inlining is not possible

There are other cases where inlining is just not possible:

1. The local function is converted to a delegate
2. The local function is generic. Even with a single use case inlining without changing the execution of the function is non-trivial.
3. The local function is marked static. The compiler has specific guarantees that such a method is emitted as static because there are many cases where other systems want to know a static method with that name will be available for invocation. Inlining would break these guarantees we've established.

Inlining raises questions ...

Local functions can have metadata attached to them like attributes. The compiler has to assume the attributes have meaning and will be inspected by other tools. What do we do with the attributes when we inline? Dropping them means we're very likely subverting other tooling that expected them. Moving them to the calling method is not possible (can conflict, changes intent of developer, etc ...).

Little items like this pop up in a lot of cases and don't have clear answers.

Overall this is not a "little risk" issue. It's an area that has significant risk points that have to be evaluated and considered.

[CodeSetting]

@jaredpar Thank you these are concrete examples that help me understand the problem.

The mention of "little risk" was clearly erroneous. I will edit my post to retract that.

I simply wish there was a way I could break up my (currently well-performing) method without killing its performance. The local methods I created have none of the issues you outlined. Unfortunately, contrary to popular belief, JIT sometimes gets it wrong. I just wish there was a very AgressiveInlining option.

I'll re-edit my original post to remove some of the incorrect assumptions.

[CyrusNajmabadi]

Unfortunately, contrary to popular belief, JIT sometimes gets it wrong.

This should definitely be brought to the attention of the jit. Real world examples will go a long way to address this.

[CyrusNajmabadi]

As an example, i had to write teh following today:

        public static TDetector GetOrCreate(
            Compilation compilation, EmbeddedLanguageInfo info)
        {
            // Do a quick non-allocating check first.  If not already created, go the path that will have to allocate
            // the lambda closure.
            return s_compilationToDetector.TryGetValue(compilation, out var detector)
                ? detector
                : Create(compilation, info);

            static TDetector Create(Compilation compilation, EmbeddedLanguageInfo info)
                => s_compilationToDetector.GetValue(compilation, _ => default(TDetectorInfo).Create(compilation, info));
        }

This nested local function, despite only being called in one location, must not be inlined by the language. It would be very bad to do so, converting the outer method body into a one that needs the space and logic for closure capture (something very expensive if elevated to the common case, when it should be the rare case).