Handle race when coloring nodes concurrently as both green and red

[Zoxc]

This fixes a race where a duplicate dep node gets written to the dep graph if a node was marked as green and promoted during execution, then marked as red after execution.

This can occur when a no_hash query A depends on a query B which cannot be forced so it was not colored when starting execution of query A. During the execution of query A it will execute query B and color it green. Before A finishes another thread tries to mark A green, this time succeeding as B is now green, and A gets promoted and written to metadata. Execution of A then finishes and because it's no_hash we assume the result changed and thus we color the node again, now as red and write it to metadata again. This doesn't happen with non-no_hash queries as they will be green if all their dependencies are green.

This changes the code coloring nodes red to also use compare_exchange to deal with this race ensuring that the coloring of nodes only happens once.

r? @ghost

[Zoxc]

@bors try @rust-timer queue

[Zoxc]

Looks fairly neutral locally.

Benchmark

Before

Before

After

Before

Before

After

Before

Before

After

Time

Time

%

Time

%

Physical Memory

Physical Memory

%

Physical Memory

%

Committed Memory

Committed Memory

%

Committed Memory

%

🟣 clap:check:unchanged

0.2478s

0.2474s

-0.15%

0.2474s

-0.17%

99.35 MiB

99.41 MiB

0.06%

99.52 MiB

0.17%

165.65 MiB

165.71 MiB

0.03%

165.74 MiB

0.05%

🟣 hyper:check:unchanged

0.1119s

0.1133s

💔 1.30%

0.1127s

0.76%

63.07 MiB

63.09 MiB

0.02%

63.25 MiB

0.29%

121.50 MiB

121.51 MiB

0.01%

121.59 MiB

0.08%

🟣 regex:check:unchanged

0.2081s

0.2058s

💚 -1.12%

0.2074s

-0.35%

81.38 MiB

81.35 MiB

-0.04%

81.55 MiB

0.20%

143.39 MiB

143.39 MiB

-0.01%

143.55 MiB

0.11%

🟣 syn:check:unchanged

0.4318s

0.4327s

0.21%

0.4308s

-0.23%

120.88 MiB

121.01 MiB

0.10%

121.46 MiB

0.48%

187.29 MiB

187.42 MiB

0.07%

187.85 MiB

0.30%

Total

0.9996s

0.9992s

-0.03%

0.9983s

-0.13%

364.69 MiB

364.85 MiB

0.05%

365.78 MiB

0.30%

617.83 MiB

618.02 MiB

0.03%

618.73 MiB

0.15%

Summary

1.0000s

1.0006s

0.06%

1.0000s

0.00%

1 byte

1.00 bytes

0.04%

1.00 bytes

0.29%

1 byte

1.00 bytes

0.03%

1.00 bytes

0.13%

[cjgillot]

Here's what's bugging me. I'm under the impression that this PR allows a node to change colour from green to red. It should not. A node has a single colour and should never change.

So my recommendation would be:

• fix the race by using proper atomics, yes;
• change the logic around no_hash nodes to mark them green iff !eval_always && all parents are green.

[rust-bors]

☀️ Try build successful (CI)
Build commit: f9b6979 (f9b6979e5b37291f050e6355d9997a871b20de55, parent: 39052daf937d46373ac29778e1b8853c52c0cc25)

[zetanumbers]

Here's what's bugging me. I'm under the impression that this PR allows a node to change colour from green to red. It should not. A node has a single colour and should never change.

So my recommendation would be:

• fix the race by using proper atomics, yes;

• change the logic around no_hash nodes to mark them green iff !eval_always && all parents are green.

Wouldn't #150156 already address these concerns?

[rust-timer]

Finished benchmarking commit (f9b6979): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-0.2%, -0.2%]	3

Max RSS (memory usage)

Results (primary -1.9%, secondary -5.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.9%	[-1.9%, -1.9%]	1
Improvements ✅ (secondary)	-5.7%	[-5.7%, -5.7%]	1
All ❌✅ (primary)	-1.9%	[-1.9%, -1.9%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 473.794s -> 471.369s (-0.51%)
Artifact size: 383.23 MiB -> 383.22 MiB (-0.00%)

[Zoxc]

I'm under the impression that this PR allows a node to change colour from green to red. It should not.

No, that's the current state of rustc. This PR prevents that.

[zetanumbers]

I honestly feel like @Zoxc often deals with his own meaning of compiler's code and not the code's actual behavior. It comes up when I try to convince him about anything.

[teor2345]

Would demonstrating what the code does help resolve this discussion?
(I understand it can be hard to find a reliable test/reproducer in concurrent code with race conditions.)

[Zoxc]

As discussed in Solving big #141540 issue and hunt for bad metadata files, I think the trait selection cache can leave nodes uncolored after execution. This means that we're not able to tell if dependencies are all green in all cases and #150156 would be insufficient to fix the race.

[rust-bors]

☔ The latest upstream changes (presumably #151881) made this pull request unmergeable. Please resolve the merge conflicts.