API for getting a deterministic key for a Compilation #57134
Conversation
|
I still haven't reviewed this. I am still planning to. |
I hope your feedback is deterministic so it would be the same now as when I originally filed this PR 😄 |
src/Compilers/CSharp/Portable/Compilation/CSharpDeterministicKeyBuilder.cs
Show resolved
Hide resolved
src/Compilers/Core/Portable/Compilation/DeterministicKeyBuilder.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/Compilation/DeterministicKeyBuilder.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/Compilation/DeterministicKeyBuilder.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/Compilation/DeterministicKeyBuilder.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/Compilation/DeterministicKeyBuilder.cs
Outdated
Show resolved
Hide resolved
src/Compilers/VisualBasic/Portable/Compilation/VisualBasicDeterministicKeyBuilder.vb
Outdated
Show resolved
Hide resolved
determinism.md
Outdated
| + public string GetDeterministicKey( | ||
| + ImmutableArray<AdditionalText> additionalTexts = default, | ||
| + ImmutableArray<DiagnosticAnalyzer> analyzers = default, | ||
| + ImmutableArray<ISourceGenerator> generators = default, |
There was a problem hiding this comment.
this slightly worries me as this implies the analyzers/generators are instantiated. that may not be desirable. @jasonmalinowski for thoughts.
There was a problem hiding this comment.
What was the representation you were expecting to see here?
determinism.md
Outdated
|
|
||
| Note: this scenario does require more work as the `Compilation` objects created | ||
| in the IDE and Compiler differ in subtle ways. Having this function though would | ||
| be motivation to clean these up such that they are equal. |
There was a problem hiding this comment.
similar to the recent discussion about how IncrementalSourceGneration is still slow if we require a compilation to be ready up front, we should consider what fixed costs any APIs we are creating mandate, and if we can avoid them. We've already talked about moving this off Compilation (or having a alternative non-Compilation entrypoint) to avoid teh cost of createing hte compilation. We should also consider how we can avoid any costs involved making the subpieces we build the key off of. Some things seems unavoidable (like reading in a source text). HOwever others may be avoidable if they can be determined from some more primitive piece of data. For example, instead of SGs and Analyzer instances, it would be beneficial on the IDE side if just hte PERef alone was sufficient to make that determination.
There was a problem hiding this comment.
I believe we have addressed this with the changes you added to my PR (thank you!). This is an area I want to keep a close eye on though particularly during API review. If we see any more I am happy to continue pushing on the perf aspect here.
There was a problem hiding this comment.
yes. i'm happy if we have SyntaxTreeKey.
src/Compilers/CSharp/Portable/Compilation/CSharpDeterministicKeyBuilder.cs
Show resolved
Hide resolved
src/Compilers/Core/Portable/Compilation/DeterministicKeyBuilder.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/Compilation/DeterministicKeyBuilder.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/Compilation/DeterministicKeyBuilder.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/Compilation/DeterministicKeyBuilder.cs
Outdated
Show resolved
Hide resolved
| writer.WriteObjectStart(); | ||
| WriteFileName(writer, "fileName", syntaxTree.FilePath, options); | ||
| writer.WriteKey("text"); | ||
| WriteSourceText(writer, syntaxTree.GetText()); |
There was a problem hiding this comment.
need cancellation token here.
| writeSubsystemVersion(writer, options.SubsystemVersion); | ||
| writer.Write("fileAlignment", options.FileAlignment); | ||
| writer.Write("highEntropyVirtualAddressSpace", options.HighEntropyVirtualAddressSpace); | ||
| writer.Write("baseAddress", options.BaseAddress.ToString()); |
There was a problem hiding this comment.
this likely needs passing in invariant culture here. we should likely do an audit of every ToString to make sure it is valid.
There was a problem hiding this comment.
Moved to "G" which should round trip across cultures. I'm verifying that now.
There was a problem hiding this comment.
Nope I was wrong about this.
src/Compilers/Core/Portable/Compilation/DeterministicKeyBuilder.cs
Outdated
Show resolved
Hide resolved
| public void Write<T>(string key, T value) where T : struct, Enum | ||
| { | ||
| WriteKey(key); | ||
| Write(value.ToString()); |
There was a problem hiding this comment.
i'm on the fence about this. i feel like perhaps for enums we just emit the integral value. the primary concern i have is uncertainty if the runtime always gives the same string value (for things like flags enums) when you call tostring across launches.
There was a problem hiding this comment.
The [Flags] case should be safe as the runtime guarantees the behavior there. From the doc comments
If the FlagsAttribute is applied and there is a combination of one or more named constants equal to the value of this instance, then the return value is a string containing a delimiter-separated list of the names of the constants.
There was a problem hiding this comment.
It wasn't 100% clear to me that the constants will always be listed in the same order, but it would be surprising if the order were ever different for a given value.
| For Each pair In basicOptions.PreprocessorSymbols | ||
| writer.WriteObjectStart() | ||
| writer.WriteKey(pair.Key) | ||
| writer.Write(pair.Value.ToString()) |
There was a problem hiding this comment.
interesting. this is .ToString on an aribtrary object. Do we know what forms this will take and if this is safe?
There was a problem hiding this comment.
this enumerates a dictionary. that may have different results across rules of hte same program as i don't believe .net guarantees deterministic order. Consider sorting things like this to ensure we're ok.
src/Compilers/Core/Portable/Compilation/DeterministicKeyBuilder.cs
Outdated
Show resolved
Hide resolved
src/Compilers/VisualBasic/Portable/Compilation/VisualBasicDeterministicKeyBuilder.vb
Show resolved
Hide resolved
Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
| internal TestableFileSystem FileSystem { get; } | ||
| internal BuildPaths BuildPaths { get; } | ||
|
|
||
| internal TestableCompiler(CommonCompiler compiler, TestableFileSystem fileSystem, BuildPaths buildPaths) |
There was a problem hiding this comment.
The problem with the property is that it attaches after command line arguments are parsed where the ctor parameter has a chance to thread it through. It's long been a problem with this setup and one that I'm slowly working my waay trhough.
ghost
added this to the 
The
Compilationtype is fully deterministic meaning that given the same inputs(
SyntaxTree,CompilationOptions, etc ...) it will produce the same output. Bythat it means it will produce byte for byte equivalent binaries and the same
diagnostics. This is extremely valuable in infrastructure because it allows
for caching and opens the door for efficient distributed processing.
At the moment this is hard to leverage because consumers can't determine if
two
Compilationinstances are equivalent for the purposes of determinism.Customers have to resort to hand written comparisons which requires a fairly
intimate knowledge of the compiler (for example knowing what does and does not
impact determinism). Such solutions are not version tolerant; every time the
compiler adds a new property that impacts determinism the solution must be
updated. Even when proper equality checks are in place this does not help
distributed computing where equality must be decided across different processes.
The motivation here is to provide an API that returns a string based key for a
given
Compilationsuch that for two equivalentCompilationinstances thekey will also be equivalent. This will allow customers who want to leverage
deterministic caching and communication to have a simple, and transmittable,
way to full describe the
Compilationthey are processing.Remaining items:
Follow up Items: