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

TopologicalSort stack overflow when decompiling long method #3075

Closed
sharwell opened this issue Sep 1, 2023 · 3 comments
Closed

TopologicalSort stack overflow when decompiling long method #3075

sharwell opened this issue Sep 1, 2023 · 3 comments
Labels
Decompiler The decompiler engine itself Performance
Milestone

Comments

@sharwell
Copy link
Contributor

sharwell commented Sep 1, 2023

The following method uses recursion instead of iteration for the call to Visit:

public List<Block> TopologicalSort(bool deleteUnreachableBlocks = false)

This will stack overflow when decompiling the following property:
https://github.com/microsoft/fluentui-blazor/blob/9d092562af1570269c62be9a99b6e7e9a7470c87/src/Assets/FluentUI.Icons/Icons/Icons.cs#L14

The property has the following general structure:

public static IEnumerable<IconInfo> AllIcons
{
    get
    {
        yield return new IconInfo { Name = "Accessibility", Variant = IconVariant.Filled, Size = IconSize.Size16 };
        yield return new IconInfo { Name = "Accessibility", Variant = IconVariant.Regular, Size = IconSize.Size16 };
        // ~14500 more 'yield return' lines 
    }
}
@dgrunwald
Copy link
Member

Lots of the decompiler uses recursive algorithms. While TopologicalSort is easy enough to convert to an explicit stack; there are just too many other recursive methods; it's not reasonable to convert all of these (especially since many of them are more complicated than TopologicalSort.
The method in your assembly ends up having a dominator tree that has 15k levels, so this affects not just the initial recursion over all blocks; but also all following dominator tree traversals.

Can you run the decompiler on a larger stack instead? (e.g. using the new Thread constructor overload that takes an explicit stack size)

ILSpy.exe does not crash on your example, as we use editbin.exe to re-configure the default stack size to 16 MB. (However, ILSpy is really slow on your example, taking around ~30 minutes. We're working on a fix for that.)

@siegfriedpammer siegfriedpammer added Decompiler The decompiler engine itself Performance labels Oct 8, 2023
siegfriedpammer added a commit that referenced this issue Oct 8, 2023
…ckTransformContext.IndexOfFirstAlreadyTransformedInstruction, which allows us to track already transformed instructions after a block has been merged into another by ConditionDetection.
siegfriedpammer added a commit that referenced this issue Oct 14, 2023
…tore bit individually in ReachingDefinitionsVisitor.GetStores()
siegfriedpammer added a commit that referenced this issue Oct 14, 2023
…me in cases with a large number of local variables.
@siegfriedpammer
Copy link
Member

siegfriedpammer commented Oct 15, 2023

Decompiling class Microsoft.Fast.Components.FluentUI.Icons on a

AMD Ryzen 7 3700X 8-Core Processor 

Base speed:	3,60 GHz
Sockets:	1
Cores:	8
Logical processors:	16
Virtualization:	Enabled
L1 cache:	512 KB
L2 cache:	4,0 MB
L3 cache:	32,0 MB

yields the following results for various optimizations we performed:

using commit time (avg of 10 runs) % of original time
a9eccdb 587,6s 100,00%
0a2037a 51,2s 8,71%
09691bd 47,1s 8,02%
cef99dc 38,5s 6,55%
ee160b4 25,7s 4,37%
5a5be02 25s 4,25%

I think there's not much left we can do (for now) to improve this specific case, without making the decompiler slower in the general case.

@sharwell
Copy link
Contributor Author

@siegfriedpammer those are some impressive gains!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 22, 2024
mattsh247 pushed a commit to mattsh247/ILSpy that referenced this issue Jul 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Decompiler The decompiler engine itself Performance
Projects
None yet
Development

No branches or pull requests

4 participants