Constant propagation without iterations and stack based #1756

vitek-karas · 2021-01-14T22:44:12Z

Changes how the constant propagation works. Instead of iterating over all methods multiple times, use a stack to emulate recursive processing of methods as they are needed.

The doc in this change contains lot of details about the motivation and the algorithm used so please refer to that.

This essentially makes constant propagation and branch removal per-method and not global step. It's still left as a global step which walks all methods and processes them, but that part is now not required by the algorithm.

Interesting statistics (on a console hello world app):

Before this change we scanned over all methods 3 times - in total more than 217K method body scans). After the change we only scan over a total of 73K method bodies (total number of methods is 72K, so we scan most methods only once)
Since there are no loops in framework, the maximum number of times we scan any method is 2
The maximum depth of the processing stack is 16 (but it's a simple app)

The great savings in CPU are accompanied with a larger memory usage. The step now stores a dictionary of all methods (and one reference as a value, the values are almost all non-unique so the storage necessary for values is tiny). This will be necessary anyway for once we move this into MarkStep as it has to keep a cache of already processed methods.

Part of #1733

Add a test for the complex case

vitek-karas · 2021-01-15T15:52:24Z

Should be ready for final review - PTAL.

src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs

marek-safar · 2021-01-18T08:46:06Z

src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs

+
+				// Note that stack version is not changing - we're just postponing work, not resolving anything.
+
+				constantResultInstruction = null;


What is the logic here? Could you add comment how does the code deal with possibly two different results for same method?

I don't understand the question. Method can only have one final result. If it's still in the queue (as is this case) it doesn't have a result yet. That's why it returns false here - telling the caller that no result is available.

true, why do you need the remove/addfirst then?

To move it to the top of the stack. This is so that callees are processed before callers next time around. For example if both A->C,B and B->C... after processing A the stack would be B,C,A (first is top). After processing B without the shuffle it would remain B,C,A - and it would get stuck. So the remove/add moves C to the begining so that the stack is C,B,A - C is processed and then B can get processed and so on.

Yeah, it looks like this is enough if we don't handle recursion. I couldn't come with a case where this would break ;-)

marek-safar · 2021-01-18T08:49:30Z

src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs

+		//   - ProcessedUnchangedSentinel - which means the method has been fully processed and nothing was changed on it - its value is unknown
+		//   - NonConstSentinel - which means the method has been processed and the return value is not a const
+		//   - Instruction instance - method has been processed and it has a constant return value (the value of the instruction)
+		Dictionary<MethodDefinition, object> processedMethods;


I'm not sure the perf gain is worth the complexity. Did you consider storing the instructions result in its own cache?

The only value which we could in theory store in a different place is the reference to the queue. All the others are valid "final" results. The instruction means it's a const method and has a value (just like before). The NonConstSentinel means it doesn't have a const value - this is new but necessary. Before this change we relied on multiple iterations to react to changes. The cache only stored const values - if a method was not in the cache it could mean either "nonconst" or "don't know/yet". In this change we need to do things per-method, and so we need to store the negative "nonconst" results in the cache as well.
The only optimization here is the ProcessedUnchangedSentitel which means "processed, but didn't compute result". This is pretty important optimization. In the hello world app, there's a total off 75K methods, 67K end up in this ProcessedUnchanged state - meaning we did process them (good), but nothing needed to their value. So we avoided running the const analyzer on ~80% of the methods. I could remove this, but it feels like a good optimization.

What I meant is that the code uses the cache for two different purpose

as the final method -> Instructions? cache

as temporary processing cache

and I'm just not sure it's worth dropping to object to facilitate that

Makes sense - split it into two dictionaries.

src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs

marek-safar · 2021-01-18T08:54:13Z

src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs

+					// No such node was found -> we only have nodes in the loop now, so we have to break the loop.
+					// We do this by processing it with special flag which will make it ignore any unprocessed dependencies
+					// treating them as non-const. These should only be nodes in the loop.
+					treatUnprocessedAsNonConst = true;


Shouldn't this be stack-based for nested loops?

I think only the inner most loop will be "caught" by the detection here - all nodes having the same version. This will "break" one of the nodes in the inner most loop. Continuing processing should then completely process the inner most loop, at which point the outer loop will be detected (since it will become the inner most at that point) and so on...

I added a new tested with nested loops to validate that it doesn't break the algorithm.

src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs

marek-safar · 2021-01-18T09:17:32Z

src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs

+					}
+
+					// No such node was found -> we only have nodes in the loop now, so we have to break the loop.
+					// We do this by processing it with special flag which will make it ignore any unprocessed dependencies


The comment suggests a better name for the field, which part of the name the bool value treatUnprocessedAsNonConst does change?

Sorry - I don't think I follow...
I guess the name could mention that it's for breaking loops - but it didn't felt right to me. We are using it to break loops, sure, but what it does is in its name - it will treat all unprocessed dependencies as non-const.

I'll change it to treatUnprocessedDependenciesAsNonConst.

Let me ask differently, what does treatUnprocessedDependenciesAsNonConst = false do ?

If there's at least one unprocessed dependency, the processing of the current method will be stopped and we go back to the stack - in the code/comments I call this "backing off". The method will be retried at some point.

Don't store methods on stack in the same structure as methods which are already processed. Makes the processed methods structure more strongly typed.

sbomer

Just wanted to note - I think this has worst-case performance of O(n^2) in the number of loop methods since we can iterate over the loop methods again whenever we encounter a new call to something already on the stack. But it shouldn't matter as long as the loop cases are rare and small.

I left a couple other comments about optimizing the loop cases, but feel free to disregard them (I think readability matters more as long as we are already assuming loops are uncommon).

sbomer · 2021-01-19T17:12:52Z

src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs

+					// To fix this go over the stack and find the "oldest" node with the current version - the "oldest" node which
+					// is part of the loop:
+					var lastNodeWithCurrentVersion = stackNode;
+					for (var currentNode = stackNode; currentNode != null; currentNode = currentNode.Next) {


This will walk the whole stack . I'm not sure it matters since we expect loop cases are rare, but would lead to bad worst-case performance. A small optimization would be to walk the list backwards.

That's a good idea - changed.

sbomer · 2021-01-19T17:16:41Z

src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs

+					// Now go back over all nodes from the "oldest" one back to the top and find any nodes which are not of current version.
+					// For all of them, move them to the top of the stack.
+					var candidateNodeToMoveToTop = lastNodeWithCurrentVersion;
+					bool foundNodesWithNonCurrentVersion = false;


You could also compute foundNodesWithNonCurrentVersion in the loop above to avoid walking the loop nodes twice in case there are none.

Actually it can all be done in one backward full walk of the stack - changed the implementation.

src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs

…r#1756) Changes how the constant propagation works. Instead of iterating over all methods multiple times, use a stack to emulate recursive processing of methods as they are needed. The doc in this change contains lot of details about the motivation and the algorithm used so please refer to that. This essentially makes constant propagation and branch removal per-method and not global step. It's still left as a global step which walks all methods and processes them, but that part is now not required by the algorithm. Interesting statistics (on a console hello world app): * Before this change we scanned over all methods 3 times - in total more than 217K method body scans). After the change we only scan over a total of 73K method bodies (total number of methods is 72K, so we scan most methods only once) * Since there are no loops in framework, the maximum number of times we scan any method is 2 * The maximum depth of the processing stack is 16 (but it's a simple app) The great savings in CPU are accompanied with a larger memory usage. The step now stores a dictionary of all methods (and one reference as a value, the values are almost all non-unique so the storage necessary for values is tiny). This will be necessary anyway for once we move this into `MarkStep` as it has to keep a cache of already processed methods. Commit migrated from dotnet/linker@279c07f

vitek-karas added 3 commits January 14, 2021 14:27

Change method processing to be stack based instead of iterations

0e00326

Formatting and cleanup

41feb01

Fixes after merge with master

40b1b09

vitek-karas added the area-Linker: Steps label Jan 14, 2021

vitek-karas added this to the .NET 6.0 milestone Jan 14, 2021

vitek-karas requested a review from sbomer January 14, 2021 22:44

vitek-karas self-assigned this Jan 14, 2021

vitek-karas mentioned this pull request Jan 14, 2021

Make constant propagation and branch removal per-method, on-demand and called from MarkStep #1733

Closed

3 tasks

vitek-karas added 5 commits January 15, 2021 06:47

Added loop detection and resolution to the doc

2eed953

Implement better loop resolution

74dd51a

Add a test for the complex case

Add one possible future improvement

5af55c5

Fix mono build

9a14750

Hopefully fix Mono build for real

4ebf4f6

vitek-karas marked this pull request as ready for review January 15, 2021 15:51

vitek-karas requested a review from marek-safar as a code owner January 15, 2021 15:51

vitek-karas changed the title ~~[WIP] Constant propagation without iterations and stack based~~ Constant propagation without iterations and stack based Jan 15, 2021

marek-safar reviewed Jan 18, 2021

View reviewed changes

vitek-karas added 3 commits January 18, 2021 06:15

PR feedback

d68719c

PR feedback - refactor data structures

05fe5db

Don't store methods on stack in the same structure as methods which are already processed. Makes the processed methods structure more strongly typed.

Formatting

10db6f6

marek-safar approved these changes Jan 19, 2021

View reviewed changes

sbomer reviewed Jan 19, 2021

View reviewed changes

vitek-karas added 3 commits January 19, 2021 13:40

PR feedback

8e44aa9

Formatting

57cc82f

Delete unwanted files

86fb5bd

sbomer approved these changes Jan 20, 2021

View reviewed changes

vitek-karas merged commit 279c07f into dotnet:master Jan 20, 2021

vitek-karas deleted the ConstantPropStackProcessing branch January 20, 2021 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constant propagation without iterations and stack based #1756

Constant propagation without iterations and stack based #1756

vitek-karas commented Jan 14, 2021 •

edited

Loading

vitek-karas commented Jan 15, 2021

marek-safar Jan 18, 2021

vitek-karas Jan 18, 2021

marek-safar Jan 18, 2021

vitek-karas Jan 18, 2021

marek-safar Jan 19, 2021

marek-safar Jan 18, 2021

vitek-karas Jan 18, 2021

marek-safar Jan 18, 2021

vitek-karas Jan 18, 2021

marek-safar Jan 18, 2021

vitek-karas Jan 18, 2021

vitek-karas Jan 18, 2021

marek-safar Jan 18, 2021

vitek-karas Jan 18, 2021

marek-safar Jan 18, 2021 •

edited

Loading

vitek-karas Jan 18, 2021

sbomer left a comment

sbomer Jan 19, 2021

vitek-karas Jan 19, 2021

sbomer Jan 19, 2021

vitek-karas Jan 19, 2021


		// Note that stack version is not changing - we're just postponing work, not resolving anything.

		constantResultInstruction = null;

Constant propagation without iterations and stack based #1756

Constant propagation without iterations and stack based #1756

Conversation

vitek-karas commented Jan 14, 2021 • edited Loading

vitek-karas commented Jan 15, 2021

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

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

marek-safar Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbomer left a 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

vitek-karas commented Jan 14, 2021 •

edited

Loading

marek-safar Jan 18, 2021 •

edited

Loading