Make TaskRegistry tasks ordering deterministic (FIFO)

[JanKrivanek]

Changes extracted #8861

Context

TaskRegistry tasks order depended on implicit ordering of tasks in internal Dictionary. This can manifest only when there are multiple tasks with same name registered and task invoking doesn't specify any parameters (arch, fw) - but then it can lead to hard to investigate bugs, so worth fixing

Changes Made

Added order id to task registrations, and use it for sorting fetched matches.

Testing

Existing tests

[JanKrivanek]

@ladipro - assigning you as one of the reviewers, as you've already reviewed this thoroughly as part of #8861

[JanKrivanek]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[rainersigwald]

I'm having a very hard time understanding the implications of this. What's the user bug that could result? Can you craft a project to show that bug?

[JanKrivanek]

I'm having a very hard time understanding the implications of this.

This can be considered future proofing only.

What's the user bug that could result?

Unexpected Task being executed (would multiple tasks with same name be registered - which we do e.g. for GenerateResource)

Can you craft a project to show that bug?

Luckily not.
We rely on implicit ordering of KeyValuePairs returned from dictionary. We use Dictionary<K,P>. We only insert, never delete (AFAIK). This exact usage pattern currently give us unguaranteed ordering by insertions.
This is thanks to Dictionary enumerator: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs#L1385-L1408, returning the entries in the internal order, that during insertions with no collisions happen to be increasing only (regardless of actual hash values): https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs#L572-L606

This is very internal detail that can change in future BCL, and as well which is not held by other dictionary implementations: ConcurrentDictionary, ImmutableDictionary - we may get sneakingly broken when moving to those (this is how the problem was discovered).

[rainersigwald]

Can you combine this with #9032 and run an experimental VS insertion just to check on unexpected perf gotchas?

[JanKrivanek]

Can you combine this with #9032 and run an experimental VS insertion just to check on unexpected perf gotchas?

https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/484352

I wanted to combine with #8928 as well - but could not easily restore the branch (git push origin 137e466b34c8c7ae6bfb3d489e78fb42599bb594:refs/heads/bug/1824802.guard-TaskFactoryWrapper-dictionaries from the clone of the work didn't work as the commit sha seems to be completely gone) - so that one will go sometime later

[JanKrivanek]

Can this exp insertion outcome be interpreted as OK?:

##[warning][VS SDK Restore PR test] Pipeline run failed. 
##[warning]See the originating test pipeline to investigate: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=8075133&view=results
##[debug][DD-VS-PR-DDRIT] No test pipeline failures

The DDRIT passed - which is probably the main thing we wanted.
The VS SDK restore is failing with error: NU1102: Unable to find package Microsoft.Build.Framework with version (>= 17.8.0-preview-23364-02) - which I guess is expected case for experimental insertion that (probably) doesn't push packages - isn't it?

[rainersigwald]

@JanKrivanek exactly correct--symbol check and VSSDK test failures are expected for experimental insertions (because we don't publish those symbols or packages, because they're an experiment). Usually perf DDRITs are the reason to run one.