Ensure stable order in .deps.json file #118185
Conversation
github-actions
bot
added
the
needs-area-label
dotnet-policy-service
bot
added
the
community-contribution
| { | ||
| jsonWriter.WriteStartObject(DependencyContextStrings.RuntimesPropertyName); | ||
| foreach (RuntimeFallbacks runtimeFallback in context.RuntimeGraph) | ||
| foreach (RuntimeFallbacks runtimeFallback in context.RuntimeGraph.OrderBy(r => r.Runtime, StringComparer.Ordinal)) |
There was a problem hiding this comment.
Is there a risk of this breaking how the runtime fallback is computed/used?
There was a problem hiding this comment.
@ericstj / @ViktorHofer Can one of you weigh in on if this can affect behavior adversely?
There was a problem hiding this comment.
The host implements runtime lookup. @elinor-fung I would expect that nothing here is order dependent since the host should be probing in the order defined in the runtimes section of the shared-framework's deps file.
There was a problem hiding this comment.
The order does matter for RID fallbacks, as the host uses the order of the fallbacks as defined in the .deps.json. This can be a shared framework's deps file or that for the app itself (for example, self-contained) - both should be created by this writer.
The order of the RuntimeFallbacks objects shouldn't matter (this line), but the order of the fallbacks array (line 65) definitely does. That is, a fallback array of [ "linux", "unix-x64", "unix", "any", "base" ] needs to stay in that order and should not become [ "any", "base", "linux", "unix", "unix-x64" ].
|
|
||
| private static void AddResourceAssemblies(IReadOnlyList<ResourceAssembly> resourceAssemblies, Utf8JsonWriter jsonWriter) | ||
| { | ||
| resourceAssemblies = resourceAssemblies.OrderBy(r => r.Path, StringComparer.Ordinal).ToList(); |
There was a problem hiding this comment.
This was one of the biggest culprits, leading to unpredictable order of resource dlls in deps.json files.
There was a problem hiding this comment.
What made the earlier part of the build non-deterministic in how it passed these in? Typically the order of things in the file, the order in which the filesystem enumerates, etc is deterministic. It's when folks do things like shove them into a dictionary/hashtable then dump that back out that the order becomes non-deterministic.
I'm not too familiar with how much we resign to sorting to solve determinism problems vs rooting out unintentional disorder. Sorting is certainly a more stable solution, but it does burn cycles. I would be interested in hearing from @dsplaisted about this.
There was a problem hiding this comment.
the order in which the filesystem enumerates
this is not guaranteed though
from Linux readdir
The order in which filenames are read by successive calls to readdir() depends on the filesystem implementation; it is unlikely that the names will be sorted in any fashion.
and on Windows FindNextFileA:
The order in which this function returns the file names is dependent on the file system type. With the NTFS file system and CDFS file systems, the names are usually returned in alphabetical order. With FAT file systems, the names are usually returned in the order the files were written to the disk, which may or may not be in alphabetical order. However, as stated previously, these behaviors are not guaranteed.
There was a problem hiding this comment.
And MSBuild doesn't ensure it either? Is everything that consumes items in the build meant to sort them in order to be deterministic? @rainersigwald
I'm just imagining that we all do less work if the inputs are stable and no one needs to sort than if we all assume inputs are not stable and everyone must sort. I'd prefer a world where every component just preserves input derived order and doesn't apply its own sorting.
There was a problem hiding this comment.
I'd prefer a world where every component just preserves input derived order and doesn't apply its own sorting.
This is mostly what MSBuild does--for globs we just preserve filesystem order, and other item operations are order-preserving.
There was a problem hiding this comment.
So presumably that can be done here instead of adding all this sorting. @omajid - do you have a couple example binlogs that show where the ordering gets mixed up? (Bonus if we find and fix a determinism bug that is impacting multiple components instead of just this one).
When writing out the .deps.json file through DependencyContextCreator, sort as much information as possible to ensure a stable order of the file contents. This helps make application builds deterministic. Contributes to: dotnet/source-build#4963
omajid
force-pushed
the
deps-json-stable-order
branch
from
a0a2f74 to
beaf7ff
Compare
vcsjones
added
area-DependencyModel
and removed
needs-area-label
|
Tagging subscribers to this area: @dotnet/area-dependencymodel |
|
It would be good to measure if this will impact performance of SDK builds. cc @dsplaisted |
|
@omajid pinging on this one - conclusion was we should root out the source of disorder on a single machine in the build rather than forcing sorting. I'd be happy to work with you on this. I think we should find a case where reordering happens and then look at binlog before/after. |
ericstj
added
the
needs-author-action
|
This pull request has been automatically marked |
|
This pull request will now be closed since it had been marked |
7 participants
When writing out the .deps.json file through DependencyContextCreator, sort as much information as possible to ensure a stable order of the file contents. This helps make application builds deterministic.
Contribute to dotnet/source-build#4963