Split match exhaustiveness into two files

[Nadrieril]

I feel the constructor-related things in the _match module make enough sense on their own so I split them off. It makes _match feel less like a complicated mess. I'm not aware of PRs in progress against this module apart from my own so hopefully I'm not annoying too many people.
I have a lot of questions about the conventions in naming and modules around the compiler. Like, why is the module named _match? Could I rename it to usefulness maybe? Should deconstruct_pat be a submodule of _match since only _match uses it? Is it ok to move big piles of code around even if it makes git blame more difficult?

r? @varkor
@rustbot modify labels: +A-exhaustiveness-checking

[mark-i-m]

I have a lot of questions about the conventions in naming and modules around the compiler.

My guess is it was called _match because it's about exhaustiveness and the most obvious usecase is checking match expressions, but you can't name a module match because it is a keyword...

[Nadrieril]

@mark-i-m but there's already check_match in the same folder :/ Maybe check_match appeared later

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[varkor]

Sorry, I know this is predisposed to merge conflicts because of its size. I will try to review soon; I just need to set aside some time to look through the changes.

[Nadrieril]

@varkor No worries. I'm just moving code around so it should be quick to review. Do you have an opinion on whether I could rename _match to usefulness while I'm at it?

[varkor]

I'd be happy changing the name to usefulness.rs, considering it's not just relevant to match.

[varkor]

I have a lot of questions about the conventions in naming and modules around the compiler. Like, why is the module named _match? Could I rename it to usefulness maybe? Should deconstruct_pat be a submodule of _match since only _match uses it? Is it ok to move big piles of code around even if it makes git blame more difficult?

I also wondered about the name (other than the point @mark-i-m raised about it denoting the match keyword); I'm not sure the exact reasoning (maybe one could trace their way back through git blame to deduce something of the motivation. I think it could be less confusing as usefulness. I don't have a strong opinion on whether it ought to be a submodule, so whichever you find more convenient. It's fine to refactor if it makes the code more readable: it makes git blame a little more awkward to use, but I think code readability is more important here.

I'm happy with the changes, once the merge conflicts have been addressed.

[Nadrieril]

Fixed the conflict, and renamed _match.rs to usefulness.rs. We'll have to update the dev guide since it points to _match.

[varkor]

Upping the priority because this is liable to conflict.

@bors r+ p=1

[bors]

📌 Commit 941c6ac has been approved by varkor

[bors]

⌛ Testing commit 941c6ac with merge fd6b537...

[bors]

☀️ Test successful - checks-actions
Approved by: varkor
Pushing fd6b537 to master...

[Nadrieril]

Thanks!

[rylev]

@Nadrieril @varkor This change caused a very large performance regression in the match-stress-enum stress test. It's hard to tell from the diff what the actual changes in code were (vs simple relocation). We should probably look into what caused such a large regression.

[varkor]

Those are very significant. There weren't very many actual changes, so hopefully it should be possible to narrow down the issue. @Nadrieril: are you able to investigate this?

[Mark-Simulacrum]

Note that a very plausible answer here is "CGU changed, inlining changed" -- modules are the basis for our CGU partitioning scheme, so splitting them can have outsized effects even if you're just moving code around. Where there any actual changes made or did we just move code around?

[varkor]

It was just code being moved around, so I imagine it was something like this.

[Mark-Simulacrum]

Would be good to see if we can add a #[inline] attribute or otherwise help LLVM out a bit to mitigate the impact here.

[Nadrieril]

Ouch, I did not think that moving code could have such an impact. Will investigate

[Nadrieril]

I'm super confused: I just measured the same commit range locally and I see no difference at all. Must be something in my config.toml I guess? That's what's in it, and I deleted build entirely before starting to measure.

changelog-seen = 2
[llvm]
download-ci-llvm = true
assertions = true
[rust]
debug-logging = true
debuginfo-level-rustc = 1
incremental = true
deny-warnings = false

Could incremental be the culprit?

[Mark-Simulacrum]

There's a few differences: incremental bumps CGU counts to one-per-module IIRC, and I think might disable some of the merging that happens normally? We also lower the ThinLTO import limit to 10 (from 100?) IIRC for LLVM when compiling incrementally, which you can manually set in config.toml (thin-to-instr-limit I believe).

[Nadrieril]

I tried again and yup without incremental I can reproduce. Are you saying that incremental+thin-to-instr-limit=100 might reproduce too? Or do you think it's more probably the CGU counts stuff? I'd love a reproduction with incremental because it makes it way quicker to try things

[Mark-Simulacrum]

Hm, not sure, and I don't recall what the limit actually is by default w/o incremental unfortunately (100 is just a guess).

[Nadrieril]

config.toml.example says that the default value is 100. I tried with that and incremental and can't reproduce anymore. So it's something else. Will have to bisect without incremental then

[Nadrieril]

Unsurprisingly the commit the broke perf is the one where I move half the code to a new file. Found a fix: #79680