dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.91k stars 4.01k forks source link

Class with over 11,000 methods takes a *looong* time to build #18631

Open socmag opened 7 years ago

socmag commented 7 years ago

Version Used: 1.0.1

(Linux Ubuntu 16.04, Core i7 Skylake, 16GB)

I know this is not a "normal" kind of scenario by any means, but for good reasons I have a class with over eleven thousand methods and it takes quite some time to build, and totally kills VS Code.

I'm sure you have bigger fish to fry than caring about these kinds of fringe perf issues, but just letting you know in case you wanted to add a test like this to your test suite.

Best wishes

jaredpar commented 7 years ago

Can you give us a bit more details about the type of methods involved here? Are they overloads of each other?

gafter commented 7 years ago

@socmag Could you please provide a source file that illustrates the issue?

tamlin-mike commented 7 years ago

I found it quite easy to whip together a quite easy repro displaying a serious performance regression of csc 2.1.0.61520 compared to an older (system-installed) 4.6.1586.0 (do not be fooled by the version numbers - it's a bit like Explorer sort order, 2 is larger than 4).

Paste the following in a command prompt:

echo class foo { > foo.cs
echo static int Main(string[] argv) { return 0; } >> foo.cs
for /l %i in (1,1,2500) do @echo public int m%i(byte arg) { return arg+1; } >> foo.cs && @echo public int m%i(short arg) { return arg+1; } >> foo.cs && @echo public int m%i(int arg) { return arg+1; } >> foo.cs && @echo public long m%i(long arg) { return arg+1; } >> foo.cs
echo } >> foo.cs

The older (64-bit) 4.6.1586.0 compiles the file into an exe in 281 milliseconds (excluding 30-ish milliseconds process startup overhead).

The newer 2.1.0.61520 had around 200ms startup overhead, and then took 2500 (!) milliseconds to compile the file.

That's a compilation performance regression of almost nine times.

socmag commented 6 years ago

Oh boy, took me a long time to get back to this. Apologies.

So right, if you want to say, build your own Tuple library, and have constructors for all element types that the Tuple supports, for Tuples up to say, length 4 or 5..

Let's say the Tuple elements you support are int32, in64, bool, string, float, double and byte[].

Since C# doesn't support variadic templates you have to write a program that generates all the overloads for the constructors so I can say new Tuple(true, "hello", 3.14, ...)

That's some factorial explosion right there.

You might think you can create simply N generic constructors with Type T0, T1, ... TN, but unfortunately it seems in C# you can't dispatch to another function based on a generic type in the parent function like you can in C++.

For instance

void Foo<T>(T val)
{
  this.DoIt(val);
}

void DoIt(string val) { }
void DoIt(int val) { }
void DoIt(bool val) { }

Foo("sad face");

Doesn't compile.

That's a separate issue, and fixing that would pretty much make this issue irrelevant, but I can't see you fixing that any time soon. Or?

socmag commented 6 years ago

Going back to the inability of a generic function to dispatch to a polymorphic overriden function based on a generic input type seems like super low hanging fruit. I assume you are already flowing the type information. You have to be, surely?

I was pretty shocked it didn't work tbh.

Maybe that is the fix here and would be very helpful to a lot of people, even without variadic template support that would open up a huge can of worms.

So, feature request, or maybe a bug fix.

If you can allow a generic function to dispatch based on the concrete input type, would be super awesome :-) <3

socmag commented 6 years ago

Just as a side note, we are almost done with our platform and we really want the C# support to be as good as the JavaScript and Python support from the get-go, but even better and with the type safety of C++.

There is nothing wrong with the Tuple support in .Net per-se, but we need something much more dynamic and our internal structures are nothing like the "cons cells" approach that back up the current .Net native tuple implementation.

I apologize this issue is conflated by two different requests. Just need to make it work because right now building dynamic tuples in C# is awful.

Going beyond that and looking at the source I see other places in the .Net library itself that could really use the same mechanisms. Folding a series of generic types into a stream of concrete types.

Think immutable packed strings of heterogeneous types with the same operators that you find on string itself.

So I know this dispatch issue is not local to us.

I think we are pretty close but just a little bit more.

It is blocking.

svick commented 6 years ago

@socmag

You might think you can create simply N generic constructors with Type T0, T1, ... TN, but unfortunately it seems in C# you can't dispatch to another function based on a generic type in the parent function like you can in C++.

Yes, you can't, because that's not something that you can express in IL. Some approaches that are used to work around that:

  1. dynamic, the quick and dirty (and slow) solution:

    void Foo<T>(T val)
    {
     this.DoIt((dynamic)val);
    }
  2. A C# 7.0 typeswitch (or a series of type-checks in earlier versions):

    void Foo<T>(T val)
    {
     switch ((object)val) // cast not necessary in C# 7.1
     {
     case string s:
       this.DoIt(s);
       break;
     case int i:
       this.DoIt(i);
       break;
     case bool b:
       this.DoIt(b);
       break;
     default:
       throw new SomeException();
     }
    }

So, feature request, or maybe a bug fix.

It's not a bug, this is working as designed. If you want to propose a change to the C# language, you should create a separate issue in the csharplang repo.

But if you want it to go somewhere, be prepared to provide more details (what happens if there isn't a method to dispatch to, e.g. Foo<DateTime>()? how would the generated IL look like?).

Just as a side note, we are almost done with our platform and we really want the C# support to be as good as the JavaScript and Python support from the get-go, but even better and with the type safety of C++.

I seriously doubt a class with 11 000 methods is the right way to accomplish that.

There is nothing wrong with the Tuple support in .Net per-se, but we need something much more dynamic and our internal structures are nothing like the "cons cells" approach that back up the current .Net native tuple implementation.

.Net tuples use the "cons cell" approach only for tuples that have more than 7 elements. Why does that mean you can't use them? Especially since you said you have constructors only for up to 5 elements.

socmag commented 6 years ago

With regard to the last question, I don't want to get into the specifics in too much detail.

But let me put it this way, if a Tuple were a string and there was an arbitrary decision that string representations were packed as bytes up to length 7, then it becomes a linked list of things you'd quite rightly call me crazy.

We have a dynamic Tuple format that is immutable, is lexicographically sortable and is packed into a binary blob. Moreover it doesn't have to pack a 123 into a 32 bit slot, they can pack a lot smaller. That's a good thing.

Additionally you can iterate over them like strings but in a typesafe manner, concatenate them, sort them or do "string" like stuff on them. RegEx's etc.

I'm not saying they are better or worse, but they do something different to the built-in tuples.

You additionally hit the nail on the head when you said the built in tuples have a limit of 5. That's a pattern that is repeated many times, for instance Funcs or Actions, I could go on. It's because of the type explosion. The built in Tuples themselves?

I understand your sample, but it is passing the buck to runtime what should be known at compile time. I don't need to tell you that. Hell, I could have a constructor that takes an object[] parameter pack and just loop over them, but then why have generics in the first place? C# is supposed to better than "Java", and it is..

I'm quite confident that Roslyn can infer the types and being able to do that dispatch would be pretty easy. Wasn't that why Roslyn was built and why we have such great autocomplete these days?

The first point is fair, you are basically asking me how a reference type can be dispatched when the thing is in a boxed state.

Okay, that's a reasonable question.

But even if that is the case, you could still deal with the value types directly and automatically build the switch statement for any possible boxed types. At the call site, examine what the possible matches are and make the best guess.

Even better, we already have type guards for generics in some sense via the where clause. At least that could provide a hint. It would be better than generating 11,000 functions.

Because Here's the thing, I can build a method that takes a DateTime right now, and csc will not allow me to call it with "hello World".

If you are already figuring that out I don't see why it isn't possible?

Thanks for the link to the place for making language requests.

I'll just do it with an object[] and performance and type safety be damned 😀

socmag commented 6 years ago

Just to go further, the way the type checker in C++ works is to look forward not only backwards. That's the thing that is missing.

If I pass a DateTime and the generic function tries to call a function based on T and no function accepts a DateTime or a coercive parameter then I'm out of luck and get an error.

Riddle me this..

Fact: I can build a class of 11,000 methods where every parameter is concretely specified and it works perfectly fine.

Why not for generic types?

Answer: Something is not right in the compiler.

Hint: That's a bug, not a language feature request

If this is "by design" - it's by "stupid design", and you should go ask joncaves how to do it right.

Raise the bar!

Don't get me even started about dragging in the DLR. Wow! 🤣

socmag commented 6 years ago

I apologize.

Look I just want things to be better.

I get frustrated at times.

So sorry about that.

jmarolf commented 6 years ago

@socmag sounds like there are two problems here:

  1. Overload resolution by the compiler is taking too long. This issue can act as the tracker for improving overload resolution performance
  2. There is a need for changes to overload resolution with respect to generics. This should probably be tracked as a new issue on https://github.com/dotnet/csharplang as this would require spec changes for the C# language.
svick commented 6 years ago

Just to go further, the way the type checker in C++ works is to look forward not only backwards. That's the thing that is missing.

C# is not C++ and C# generics are not C++ templates. Each have different design goals and, as a result, different limitations.

For example, did you ever want to have a C++ DLL that contains templates? You can't do that, because each instantiation of a C++ template is pretty much compiled separately and requires the original source code.

In C#, a DLL exposing generic types is nothing special. But the cost you pay for that is that the requirements of each generic type or method are specified in its signature (and the possible constraints are fairly limited).

What you're asking for is not a simple bug fix, it's a whole new language feature, which would need to be properly designed and might even require runtime changes.

gafter commented 6 years ago

The Roslyn compiler has a few disadvantages compared to the native compiler, mainly due to having to do much more (e.g. supporting a semantically rich persistent model of the program) on a managed platform. We make up for that by using concurrency to compile separate classes in parallel. This particular example is the worst case for the compiler - since there is only one class, it is processed sequentially.

tamlin-mike commented 6 years ago

@gafter Actually, it's not only limited to straight compiling, but also making MSVS horribly slow for large files.

It could be seen as me hijacking this issue for something unrelated, but it is related.

If properly designed, MSVS (with Roslyn support) would have allowed partial recompilation of just the parts of a .cs source file modified. Instead the whole shebang kicks off for pretty much every keystroke, even if it's only to change a constant initializer in a local variable (where I hope we can agree the ONLY recompilation required would be the scope).

IMO Roslyn + MSVS are inherently mis-designed (and since this is by design, it's nothing anyone can do about it), making it (literally) impossible to solve this while MS are in charge of "it" and insist "This Is The Right Way" (even when proven wrong).

I'm not saying this is a Roslyn-induced design problem; you guys are just the most obvious target right now since you took the bait, hook and sinker without even thinking through the long-term consequences. But since you are part of the problem, and Your Leadership seems to be totally immune to even valid criticism, it's pretty much Game Over for even opening a dicscussion on how to actually solve this.

But to throw you a bone, this can be solved. Speak to the C++ compiler team, esp. "minimal compilation". Read up a few lines about the word "cache".

No amount of asynch or threaded work can make up for these design errors. It needs to be solved at the core. The workaround of compiling several files in parallel is like stating "make -j solves that!", when the error is really in the parser -- it needs to be re-designed for scenarios like this. Think "DAG - for a single source file". Then it would become possible to have actual multi-threaded compilation based on (type) dependencies, not the crude per-file.

Is MSBuild a hindrance here? Most likely, but only a mouse would cower under the threat of the mighty (byggy, ununderstandable, bloated, and inherently broken) MSBuild.

I'm sorry if I come across as arrogant and aggressive, but MS have succeeded in failing to grasp even the tiniest of straws here, for many, many years, to make Roslyn work large-scale. Features, features, features, has been the words, without a single thought to scalability - on many levels.

jaredpar commented 6 years ago

I'm sorry if I come across as arrogant and aggressive,

Instead of being sorry, consider taking extra time to craft statements in a more cordial manner.

without a single thought to scalability

This is simply a false statement. Any query of our closed PRs, issues or even general community shows that scalability and perf is an incredibly important requirement of our team.

jmarolf commented 6 years ago

even if it's only to change a constant initializer in a local variable (where I hope we can agree the ONLY recompilation required would be the scope).

Because changes within method bodies cannot effect the semantics of anything out of the methods scope, roslyn does optimize for this and only re-analyzes the method body, not the entire program.

But to throw you a bone, this can be solved. Speak to the C++ compiler team, esp. "minimal compilation". Read up a few lines about the word "cache".

There are many caches in Roslyn, Several of them sit above the compiler layer in the workspace layer but that does not mean this approach was not considered (and taken).

gafter commented 6 years ago

@tamlin-mike Contrary to your assertions, the Roslyn team very carefully considered the tradeoffs between incremental compilation and performance to reach the current design point. One of the features of the current compiler is that it provides a persistent and immutable view of the program through the public APIs. That was an important feature for many clients, and enabled concurrency in the IDE and many other important clients. It means that you get very fast incremental performance when only small parts of the program are modified, if you use the compiler APIs to query about limited regions of the program (e.g. reparsing is very fast while typing, and we can produce the semantic errors for the changed region very quickly). But compiling the whole program incrementally while maintaining the persistent data structures required for our APIs would be very very expensive - if we tried to do that we still would not have been capable of shipping V1 of Roslyn. Don't imagine that we have ignored the design exercise - we actually do know how to do it (read my PhD thesis for details) - it is just very expensive.

gafter commented 6 years ago

To add to that - if Visual Studio is slow for such large files, that is something that we are likely to be capable of addressing within the current design, because the current design anticipated those kinds of clients. However it may require some performance tuning in the hot spots for this scenario. That's why this issue remains open; we intend to do that when it rises in priority over other things we could be doing.

sharwell commented 6 years ago

@socmag Does your class declare and/or use a large number of generic extension methods? If so, it may be similar to another situation that came up a couple of months ago.

CyrusNajmabadi commented 6 years ago

when the error is really in the parser

Note: the parser is extremely scalable and is designed to do minimal work in nearly all cases when an edit happens.

MS have succeeded in failing to grasp even the tiniest of straws here, for many, many years, to make Roslyn work large-scale.

This is really not true. There are many parts of roslyn that have been designed from the beginning to be extremely scalable.

However, you're running into a problem that all projects face, even when they've been designed for scalability: there are still limits and efforts still are put to the coding patterns seen by 99.999% of users, and not the ones that hit by 0.001% of them.

In other words, 11k methods is an absolutely staggeringly extreme outlier. So extreme that I would consider it irresponsible for people on the team to spend excessive efforts on it when that same effort could be spent on so many more scenarios that are routinely hit by millions of users on a daily basis.

--

Note: this attitude is not unique to roslyn. I've hit scalability issues on every single compiler i've ever used. C++ (gcc, clang, msvc) are not immune to this, and it's trivially easy to create constructs for them that absolutely bring them to their knees.

--

Finally, i would say this: Roslyn is an open source project with many contributers inside and out of MS working to improve it and make it great for their needs. If this truly is something that you think is really important, and you really want to see it improved, i would suggest diving into roslyn, learning a bunch about it, and helping improve the scalability in the ways you'd like to see done.

StingyJack commented 6 years ago

This is slightly anecdotal, but a few years ago I was working out a code generator for types derived from an existing SQL database, including constants for fields, dataset/table schema builders, POCO<-> DataRow, etc. There were >1500 tables, and CRUD procs for each, along with a few hundred other procs/views/functions/etc, and different things would generate into their own files. (Before anyone suggests "y not EF", pls attempt to load it for a schema 1/10th as large without it crashing several times)

Some of the files that were generated would get quite large, and I found that VS 2010/2012/2013 (i forget now) would more or less hang when asked to compile a single class file with more about 100KLOC . Probably ran out of memory trying to read the file.

Trying to split the methods up into partial class files allowed VS to deal with the files a bit better. But I had to eventually break up some of the methods too. as I found it couldnt compile some of the methods reliably that had more than 30-40KLOC (effectively giant switch statements for building datatable schemas).

11K methods staggering? probably not really as much of an outlier when code generating, but it is something that can be addressed by a bit of reorganization. I'd be curious to know if there is a hard limit to some of these

CyrusNajmabadi commented 6 years ago

I'd be curious to know if there is a hard limit to some of these

I don't believe there are hard limits on any of these. It's just that very few parts of the system are designed to scale to those extremes. And all you need is a single piece of code in the entire stack that doesn't scale, and you can have a major negative effect on large swaths of the experience. For example, if incremental parsing doesn't work well here, then any edits will incur a huge cost just to make a new tree. And if getting a new tree is super expensive, that will impact every editing scenario.

Similarly, semantic analysis has huge numbers of algorithms in it. Even if an algorithm is linear, then throwing 40kloc at htem may just end up taking a lot of time and impacting other parts of the experience.

If you run into these issues today we have systems and tools to allow you to provide us with detailed perf information. That information can then be used to drive understanding of what the root cause is, which may lead to changes in those systems if it is deemed worth it.

CyrusNajmabadi commented 6 years ago

You can use https://github.com/dotnet/roslyn/wiki/Reporting-Visual-Studio-crashes-and-performance-issues to report perf issues.

Thanks!

PathogenDavid commented 5 years ago

@socmag

I understand your sample, but it is passing the buck to runtime what should be known at compile time.

Sorry for reviving such an old issue. You've probably moved on from this by now, but I was surprised nobody mentioned that the JIT will optimize away the unused cases when you have type-specific branches in a generic method. One caveat is you can't use pattern matching, but it sure beats generating 11,000 methods or taking the performance hit from using an array of objects.

public static void GenericMethod<T>(T value)
{
    if (typeof(T) == typeof(int))
    {
        Test((int)(object)value);
    }
    else if (typeof(T) == typeof(float))
    {
        Test((float)(object)value);
    }
    else if (typeof(T) == typeof(string))
    {
        Test((string)(object)value);
    }
    else
    {
        throw null;
    }
}

public static void Test(int i)
    => Console.WriteLine($"Integer: {i}");

public static void Test(float f)
    => Console.WriteLine($"Float: {f}");

public static void Test(string s)
    => Console.WriteLine($"String: {s}");

The code generated by the JIT:

Program.GenericMethod[[System.Int32, mscorlib]](Int32)
    L0000: sub rsp, 0x28
    L0004: call Program.Test(Int32)
    L0009: nop
    L000a: add rsp, 0x28
    L000e: ret

Program.GenericMethod[[System.Single, mscorlib]](Single)
    L0000: sub rsp, 0x28
    L0004: vzeroupper
    L0007: call Program.Test(Single)
    L000c: nop
    L000d: add rsp, 0x28
    L0011: ret

You can see this for yourself on SharpLab.

I'd also expect the calls to Test to be inlined, but I did not check. (SharpLab suppresses JIT inlining from what I can tell.)


Also if your main concern is the lack of a compile-time error when you use an invalid type, I'd recommend creating a Roslyn analyzer.

lsoft commented 3 years ago

With the introduction of c # Source Generators into the practice, the "issue" of super-classes compilation speed will become widespread (simply because such code is easier to generate). I think everyone understands that the Roslyn development team is doing everything appropriate to speed up compilation. But the developers of source generators need recommendations for optimal code generation in terms of compilation speed. Are there any performance guidelines for optimal organizing code in compilation units, classes and methods (etc)?

In my case, I have a class that contains 2000 nested classes (135L lines of code). Compiling this file takes about 40 seconds on my laptop, which which looks suboptimal for me (or I'm wrong? how can i figure it out?). I can experiment, trying this or that code organization, but I would like guidelines so as not to go over in vain.

Thanks!

jnm2 commented 3 years ago

@lsoft The best place to ask for general guidelines is to open a new discussion at https://github.com/dotnet/roslyn/discussions/new.