rustc_middle: parallel: TyCtxt: remove "unsafe impl DynSend/DynSync"

[safinaskar]

rustc_middle: parallel: TyCtxt: remove "unsafe impl DynSend/DynSync"

We don't need to "short circuit trait resolution", because DynSend and DynSync are auto traits and thus coinductive

cc "Parallel Rustc Front-end" #113349

r? SparrowLii

@rustbot label: +WG-compiler-parallel

(rustbot sometimes ignores me and doesn't attach labels on my behalf. rustbot banned me?)

[SparrowLii]

Thanks!

@bors r+

[bors]

📌 Commit 8eba29a has been approved by SparrowLii

It is now in the queue for this repository.

[safinaskar]

@SparrowLii , I'm writing another PR, which will turn RwLock into enum { S(parking_lot::RwLock), U(RefCell) }. Is this ok? Does it have chances of being accepted?

[SparrowLii]

Actually, I'm not sure. The efficiency of RwLock is not slow in single-thread, and the enum needs runtime matching, which may affect the optimization effect.
My advice is writing a PR draft and test it with the perf tool.

[Zoxc]

Not having these short circuits regresses compile time.

[SparrowLii]

@Zoxc You mean this increases the build time of the rustc itself? I think this should be acceptable, as we can check if the TyCtxt contains non-shared structures by removing the unsafe impl here.

[Zoxc]

That's already checked. There's no reason to add 7 seconds of compile time to every rustc build.

[SparrowLii]

How can we guarantee that in the future someone won't incorrectly modify GlobalCtxt to contains non-shared data structures

[Zoxc]

There are asserts that GlobalCtxt is appropriately Send/Sync, so we don't need to check TyCtxt which only contains a pointer to it.

[SparrowLii]

We do have fn _assert_tcx_fields to check &GlobalCtxt: Dynsend/Dynsync . And unsafe impl DynSend/DynSync for TyCtxt will make this check invalid

[Zoxc]

You got that the wrong way around. _assert_tcx_fields ensures the safety of the unsafe impl. It's the reason that function exists.

[safinaskar]

@Zoxc , I think we should have as few as possible unsafe in the code. Even if this means additional 7 seconds for self-build. If the only reason for _assert_tcx_fields is ensuring safety of unsafe impl, then _assert_tcx_fields should be removed, too

[SparrowLii]

@Zoxc Ah, it makes me wonder why _assert_tcx_fields doesn't cause the rustc build time to increase. I think it should take the time to check that TyCtxt is a shared type

[Zoxc]

It does add an extra check, but it also avoids the checks on other uses of TyCtxt, since it will quickly find these impls instead.

[SparrowLii]

OK. But it's hard for me to say that using `unsafe impl' is necessary to save some build time, since the average build time for rustc is around 15 minutes.

On the other hand, I think there should be cache for each solved trait to avoid extra 7 seconds of check time. This is probably where trait solver should be optimized.