Change the build client heuristic for using a running server process

[agocke]

This is DevDiv bug 1136957. Right now we pull the file path of the running process and compare it to the file path where we expect to find vbcscompiler.exe. If the two are the same, the client uses the existing process.

However, if the server lives in a junction or some other NTFS reparse point, the resolved path of the process may be different from the path where the server is found. Instead of comparing the paths, the client will now compare the size of the files and the last write time at each of the locations. If both values are identical, that will be considered good enough to use the existing process.

@jaredpar @AlekseyTs @gafter @VladimirReshetnikov @VSadov

[gafter]

Is there some way we could resolve the file path so that we don't need a heuristic?

[jaredpar]

Why are we not using a more deterministic measure here like the checksum of the assembly file contents? That would take it from hueristic to definitive.

[agocke]

@gafter I couldn't find an API which was guaranteed to work across all NTFS reparse points.

[agocke]

@jaredpar I tried that first and ran into a couple issues

1. Since the problem we're trying to solve is actually whether two files are the same on disk, not just identical, checksum is still only a heuristic method. It's better, but how much better I'm not sure.
2. Checksuming significantly affected the performance of the CompilerServerTests, which start many independent servers to test them in parallel.
3. One of the analyzer load/unload tests consistently ran into some file locking issue. I didn't investigate further, but if that is the only blocking issue I can continue looking into it.

[jaredpar]

@agocke the compiler server needs to have the same level of reliability as a command line compilation. I don't think we should be trusting our reliability to a hueristic such as file size. It's too risky.

I disagree that checksum is a hueristic here. It gives us a strong guarantee that two compiler servers will have the same behavior. Much more than file size.

If there is a perf issue then we should be rethinking the approach vs. picking a less reliably mechanism.

[agocke]

@jaredpar Assembly identity + file version?

[jaredpar]

@agocke that would be identical between two different builds of our dogfood bits correct?

[agocke]

@jaredpar Yes.

Did we ever start encoding a hash of the assembly contents in the MVID of the assembly?

[jaredpar]

@agocke only when -deterministc is passed which we don't do here.

Have we considered dropping a file in the same directory called version.txt which contains a GUID we rev on every build?

[agocke]

@jaredpar So now when you copy the binaries you have to carry around this version.txt file too?

[jaredpar]

@agocke yes. The sharing logic would change to the following (in order):

• Does the toolset have the same path as the running VBCSCompiler
• Does the toolset have the same value inside version.txt as the running VBCSCompiler

[agocke]

@jaredpar It could work, but it seems like a giant pain.

Maybe we should consider doing something platform dependent and just having different codepaths on Linux/Mac. If I can use a native SHA1 implementation things may be faster.

[jaredpar]

@agocke we're already copying around 9+ files, how is copying around one more a pain? Seems like one more line in a script.

I don't want to add platform dependent code here. I'd take the perf hit of checksums long before platform dependent code.

I still don't quite understand why we've pushed back on checksums. If we implement it as a mitigation in the cases where paths don't match is it really that much of a perf hit? If so what is the perf hit? It would have to be pretty large for me to want to use platform specific logic here.

[agocke]

@jaredpar I'm warming up more to checksums.

The problem with adding another file is that there's always some tool which copies Roslyn that we forget to update until it breaks the world.

Let me take another crack at checkums.

[gafter]

The MVIDs should always be distinct for different binaries, even if they're not deterministic.

[agocke]

@jaredpar It sounds like the MVID is a good candidate then. Do you agree?

[jaredpar]

@agocke works for me.

[tmat]

Yes MVID is designed for this purpose. The debugger heavily relies on its uniqueness, for example.

[pharring]

In theory, you can use the FILE_ID from GetFileInformationByHandleEx.

Cracking the MVID isn't cheap. And enumerating processes still bothers me.

[tmat]

Is similar API available on Linux? If so then we should ask the BCL team to include a managed wrapper in CoreCLR profile. There are many cases when apps need to check whether given paths identify the same file. There is currently no x-plat way of doing that.

[VSadov]

I was wondering if it is possible to get a ProcessStartInfo off an already running process and treat the FileName as the "launch path" of an executable? This way the path does not need to be truly canonical, we only need to be self-consistent.

[agocke]

Talked about this offline with Paul. We're both a bit worried by the perf hit of rummaging through the binary for the MVID for each possible VBCSCompiler process.

The suggestion is to instead hash the directory the client lives in and use that as the pipe name. This has a couple benefits:

1. It solves the original problem that calling into a junction starts a new process every time. The issue is that the server process path is not the same as the client process path. Here, as long as you call the client the same every time you get the same path.
2. Now we don't have to look through processes or rely on the server information at all.
3. A performance penalty is only paid on the worst case scenario where you call into the same client through two different reparse points, but the performance penalty is at most one extra server for each reparse point. If no reparse points are present, there should actually be a performance boost due to not looking through machine processes.

[gafter]

@agocke @pharring Clever and nice solution.

[gafter]

@agocke what is the next step for this? Is this PR obsolete? Do you want me to prepare a PR for the solution above, or are you working on it?

[agocke]

@gafter Sorry, the PR is still active, just adding a new commit. Was still working on it, got sidetracked.

[gafter]

It looks like this will compute the SHA hash twice. Can we do it once, please?

[pharring]

LGTM (modulo the build failure)

[agocke]

Manually canceled because things were taking a while. I’ll retry after a merge to HEAD.