Lock coverage files

[smoelius]

I have observed corrupted rustfix coverage files while testing Clippy, and I suspect concurrent writes to be the cause.

This PR proposes a method for locking the coverage files when writing to them.

Since Clippy uses compiletest-rs, my real interest is in having the change merged into that repo. This is an "upstream" of an analogous PR there: Manishearth/compiletest-rs#259

[rust-highfive]

r? @jyn514

(rust-highfive has picked a reviewer for you, use r? to override)

[the8472]

This probably isn't good for test throughput. Can't a separate file name be reserved for each test so that there are no conflicts?

[smoelius]

Can't a separate file name be reserved for each test so that there are no conflicts?

I'm not sure how that would work. One coverage file is created for each "build base" directory. The files' contents are essentially the paths of the test files that should have a // run-rustfix annotation, but do not. It's not clear to me how one would spread this over multiple files.

[jyn514]

Since Clippy uses compiletest-rs, my real interest is in having the change merged into that repo. This is an "upstream" of an analogous PR there: Manishearth/compiletest-rs#259

Clippy and rust-lang/rust use different versions of compiletest and there are no plans to merge them. Have you observed this issue in rust-lang/rust? Or only in the clippy repo?

[smoelius]

Clippy and rust-lang/rust use different versions of compiletest and there are no plans to merge them. Have you observed this issue in rust-lang/rust? Or only in the clippy repo?

Only in the Clippy repo.

The reason for this PR is this: https://github.com/Manishearth/compiletest-rs#contributing

Please consider submitting your patch to the upstream source instead, as it will be incorporated into this repo in due time.

E.g., I thought I was making a mistake by opening a PR on compiletest-rs without first opening one here.

[jyn514]

Ah, gotcha. I think we should change that text in the clippy repo then (cc @Manishearth, @oli-obk). I don't think it makes sense to merge the change here, though.

[Manishearth]

@jyn514 in general we don't want compiletest-rs to diverge too much from upstream; which is why we try to get the changes on both sides.

As for "no plans to merge", there have been plenty of plans to merge; it's hard to get all the stakeholders on board. The status quo isn't really sustainable. compiletest-rs tries to stay close to upstream where possible, with sporadic updates.

[jyn514]

@Manishearth the last update I heard on this was https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/compiletest.20rewrite, where @oli-obk said

flip1995: But there were some blockers I don't remember in detail and the conversation about this pretty much died, because maintaining compiletest out of tree would be too high of a maintenance burden (IIRC).

oli: yes, I tried it for clippy a while ago, again for miri, and now I have given up

If you or someone on clippy would like to integrate the two repos, I'd love to help and be a part of the conversation. But I don't think we should merge changes upstream just because we hope these two codebases will get merged someday.

[jyn514]

If you or someone on clippy would like to integrate the two repos, I'd love to help and be a part of the conversation.

Frankly I'm kind of frustrated that people are making decisions based on transient comments that aren't in any issue or wiki; I've heard several times that the work is hard but I have no idea why it's hard or even what it involves.

[Manishearth]

the last update I heard on this was https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/compiletest.20rewrite,

(This is news to me, fwiw, but I can see where it's coming from)

If you or someone on clippy would like to integrate the two repos, I'd love to help and be a part of the conversation. But I don't think we should merge changes upstream just because we hope these two codebases will get merged someday.

No, that's not the reason, the reason is that keeping rustc-compiletest and compiletest-rs as close in sync as possible heavily reduces the burden on compiletest during syncs, while not being a major cost upstream.

The policy of requesting changes upstream (and upstream generally being willing to merge) far predates any talk of moving rustc-compiletest out of tree.

If you or someone on clippy would like to integrate the two repos, I'd love to help and be a part of the conversation.

I think the current blocker is that some compiler team members do not want to have to work in a separate repo to make changes to compiletest. Personally I feel like that's mitigatable with subtrees or a plugin framework, and to some extent just an acceptable price to pay to not have the maintenance mess we have now.

It's a matter of getting everyone on the same page, and at this point IMO it's quite understandable that miri/clippy maintainers are happier to do their own thing rather than collaborating; this is not a new discussion and the general experience has been that rustc maintainers aren't really willing to move forward.

What the work involves is getting everyone on rust-lang/rust comfortable with having compiletest-rs out of tree. This is all documented in an issue, there are comments from multiple rust-lang/rust maintainers (including you) in #82946 pushing back against having to work on compiletest-rs out of tree.

Once rustc maintainers are comfortable with something like that, then we can talk about the actual work involved: my understanding is that some of it is due to rustc-compiletest relying on rustc internals like libtest (to get the formatting right). There was work in the past on publishing libtest on crates.io as well.

are making decisions based on transient comments that aren't in any issue or wiki

I don't really understand what you're referring to here.

[Manishearth]

Anyway, regarding the issue at hand here: I do see that there's potentially a perf issue here; which is an okay reason not to merge this. As far as I can tell there's no reason rustc shouldn't hit this issue as well; it's just that Clippy has a lot of rustfix tests and probably hits this more often. It would be nice to come up with a solution here that doesn't cause a perf hit (if this actually does have that effect, which I'm not convinced of!)

[Mark-Simulacrum]

I am a little confused by the changes in this PR -- to me,

  let mut coverage_file_path = self.config.build_base.clone();
  coverage_file_path.push("rustfix_missing_coverage.txt");

looks like the coverage file is essentially a global file, which means that if we want to avoid clobbering it + have good performance, the right thing to do would probably be to keep a single file handle open (perhaps behind a mutex, but I not necessary so long as we're careful to ensure writes are always a single syscall?) and do writes to that single file handle?

Alternatively, keeping a global Mutex<Vec<PathBuf>> that we serialize at the end of the run seems like it would also work fine here, and not have major performance implications given the overhead of process startup and the lack of time holding that lock.

[smoelius]

@Mark-Simulacrum

the right thing to do would probably be to keep a single file handle open (perhaps behind a mutex, but I not necessary so long as we're careful to ensure writes are always a single syscall?) and do writes to that single file handle?

Is that not what this would do?
https://github.com/smoelius/rust/blob/18e4e06b4e56331cc51b658c44011c328eda7505/src/tools/compiletest/src/runtest.rs#L79
I.e., one File is created for each path, and that same File is used whenever the path is referenced.

Alternatively, keeping a global Mutex<Vec<PathBuf>> that we serialize at the end of the run seems like it would also work fine here, and not have major performance implications given the overhead of process startup and the lack of time holding that lock.

I considered this, but I couldn't see how to do it without changing compiletest's public API. Of course, that could be just a failing on my part.

[Mark-Simulacrum]

I think changing the public API seems OK, though it would probably also work to stick the file handle or a Vec into the Config struct (with a Drop impl) -- I think the general solution here seems OK, in the sense that I wouldn't expect the additional push to a mutex or even file write to have a major effect on test throughput; either operation likely pales in comparison to a test run of any kind and we hold the lock for such a short period of time that I'm not too worried about contention in practice.