Put `InhabitedPredicate::NotInModule` earlier in disjunction since it can be a lot faster by steffahn · Pull Request #158819 · rust-lang/rust

steffahn · 2026-07-05T15:30:25Z

In my local testing, a simple reordering here can help a lot already in the regression reported in #141006, speeding up compilation roughly 7-8x, so I'd like to find out if this has any performance downsides.

rustbot · 2026-07-05T15:30:29Z

r? @khyperia

rustbot has assigned @khyperia.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: compiler, types
compiler, types expanded to 75 candidates
Random selection from 22 candidates

hanna-kruppe · 2026-07-05T15:48:15Z

@bors try @rust-timer queue

Put `InhabitedPredicate::NotInModule` earlier in disjunction since it can be a lot faster

steffahn · 2026-07-05T15:51:06Z

In fact, for the crate in #141006 the really problematic/long parts seem to be completely gone like this:

(the upper graph is with this PR; I have roughly marked corresponding sections AFAICT, and resized the windows to match time scales)

rust-bors · 2026-07-05T17:59:20Z

☀️ Try build successful (CI)
Build commit: f0c1ec5 (f0c1ec5990afff289c625c2921310d86a470e7e9)
Base parent: 4eb6253 (4eb62535fc12dd1a63fd43a4173e224e79313c4d)

rust-timer · 2026-07-05T18:44:15Z

Finished benchmarking commit (f0c1ec5): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 0.6%, secondary 2.3%)

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

	mean	range	count
Regressions ❌ (primary)	1.8%	[0.8%, 2.6%]	6
Regressions ❌ (secondary)	3.1%	[1.4%, 4.4%]	6
Improvements ✅ (primary)	-2.9%	[-5.2%, -0.7%]	2
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	0.6%	[-5.2%, 2.6%]	8

Cycles

Results (secondary -2.4%)

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)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 489.956s -> 490.266s (0.06%)
Artifact size: 393.31 MiB -> 393.77 MiB (0.12%)

steffahn · 2026-07-05T22:21:36Z

I’m no expert on interpreting these results, but it looks like it doesn’t do any harm.¹ So I guess we should land this change?

Is there any practice of creating new benchmarks (or tests?) for cases like this, to prevent regressions in the code pattern that is better supported now? If so, where would that go? (If that’s a desirable thing, I’d then work a bit on trying to minimize the reproducing crate/example into something more standalone that can still demonstrate the speedup.)

And also in theory it shouldn’t possibly have any strong negative impact anyway, since the visibility checks - unlike the recursive cases that could otherwise have been first in the disjunction - should always run in constant time. ↩

khyperia · 2026-07-06T05:50:38Z

neat, this seems fine to merge, why not I suppose! let me know if you're happy with this as-is.

Is there any practice of creating new benchmarks (or tests?) for cases like this, to prevent regressions in the code pattern that is better supported now?

I don't know the context of adding benchmarks here, but it does seem nice to try to track this a bit better. From what you said in the issue

That’s not a general solution of course [nor does it introduce any caching], but it might help to some degree already.

this change might make it more difficult to find/track things, since we can't use the real-world crates out of the box anymore, since the underlying issue is "solved" by this, so we'll need some sort of repro that's all pub or whatever. Adding benchmarks/doing investigation/etc. as followup seems fine to me though, that's not a reason to not merge this.

steffahn · 2026-07-06T07:02:26Z

I agree, those steps can make sense as a follow up. From my POV, this could be merged as-is.

khyperia · 2026-07-06T07:15:05Z

@bors r+

rust-bors · 2026-07-06T07:15:08Z

📌 Commit 54cfcdf has been approved by khyperia

It is now in the queue for this repository.

panstromek · 2026-07-06T09:51:21Z

Is there any practice of creating new benchmarks (or tests?) for cases like this, to prevent regressions in the code pattern that is better supported now?

Yes. This is a candidate for secondary stress test. If you could isolate a small program that reproduces the issue and shows the 7-8x improvement, we could add it. This seems subtle enough that I think it's worth adding, but maybe others from the wg have a different opinion.

In any case, while we still don't have a test/benchmark elsewhere, I'd at least add a comment that the ordering choice is important.

Rollup of 24 pull requests Successful merges: - #158377 (add `-Zforce-intrinsic-fallback` flag) - #158642 (Clarify some interning details) - #158694 (Positive test for closures needing expectations) - #158743 (Look for cdb location in the registry first) - #158775 (bootstrap: only encode RUSTFLAGS when a flag contains a space) - #158782 (Add and use cfg(target_has_threads) to enforce no_thread impl usage) - #158785 (hook intrinsic-test into aarch64-gnu) - #158819 (Put `InhabitedPredicate::NotInModule` earlier in disjunction since it can be a lot faster) - #157734 (Stabilize `local_key_cell_update`) - #158183 (std: allocate less memory in `current_exe` for OpenBSD) - #158310 (Remove unexpected usage of Unambig in non-infer variants) - #158671 (Move tests batch 17) - #158730 (Update `FIXME(static_mut_refs)` comments) - #158752 (Reorganize `tests/ui/issues` [18/N]) - #158755 (Use `ThinVec` more in the AST) - #158757 (Fix incorrect tracking issue for `read_le`/`read_be`) - #158765 (Fix ICE on non-ident path in `doc(auto_cfg values)`) - #158771 (library: expand HashSet::extract_if coverage) - #158772 (rustc-dev-guide subtree update) - #158776 (fix: emit diagnostic for AVR target without target-cpu) - #158786 (Add regression test for builtin attr macro values) - #158810 (Add supplementary information for get_unchecked(mut)) - #158825 (Fix typo) - #158838 (tidy: Use `empty_alternate = true` for triagebot mention glob check)

Rollup merge of #158819 - steffahn:uninhab-module-check-first, r=khyperia Put `InhabitedPredicate::NotInModule` earlier in disjunction since it can be a lot faster In my local testing, a simple reordering here can help *a lot* already in the regression reported in #141006, speeding up compilation roughly 7-8x, so I'd like to find out if this has any performance downsides.

steffahn · 2026-07-06T23:37:10Z

@panstromek

Yes. This is a candidate for secondary stress test.

Thank you for the pointer. Now I’ve just got to produce a reproducing example in the first place 😇

I'd at least add a comment that the ordering choice is important

Sounds reasonable, I’ll include that in a follow-up ^^

Rollup of 24 pull requests Successful merges: - rust-lang/rust#158377 (add `-Zforce-intrinsic-fallback` flag) - rust-lang/rust#158642 (Clarify some interning details) - rust-lang/rust#158694 (Positive test for closures needing expectations) - rust-lang/rust#158743 (Look for cdb location in the registry first) - rust-lang/rust#158775 (bootstrap: only encode RUSTFLAGS when a flag contains a space) - rust-lang/rust#158782 (Add and use cfg(target_has_threads) to enforce no_thread impl usage) - rust-lang/rust#158785 (hook intrinsic-test into aarch64-gnu) - rust-lang/rust#158819 (Put `InhabitedPredicate::NotInModule` earlier in disjunction since it can be a lot faster) - rust-lang/rust#157734 (Stabilize `local_key_cell_update`) - rust-lang/rust#158183 (std: allocate less memory in `current_exe` for OpenBSD) - rust-lang/rust#158310 (Remove unexpected usage of Unambig in non-infer variants) - rust-lang/rust#158671 (Move tests batch 17) - rust-lang/rust#158730 (Update `FIXME(static_mut_refs)` comments) - rust-lang/rust#158752 (Reorganize `tests/ui/issues` [18/N]) - rust-lang/rust#158755 (Use `ThinVec` more in the AST) - rust-lang/rust#158757 (Fix incorrect tracking issue for `read_le`/`read_be`) - rust-lang/rust#158765 (Fix ICE on non-ident path in `doc(auto_cfg values)`) - rust-lang/rust#158771 (library: expand HashSet::extract_if coverage) - rust-lang/rust#158772 (rustc-dev-guide subtree update) - rust-lang/rust#158776 (fix: emit diagnostic for AVR target without target-cpu) - rust-lang/rust#158786 (Add regression test for builtin attr macro values) - rust-lang/rust#158810 (Add supplementary information for get_unchecked(mut)) - rust-lang/rust#158825 (Fix typo) - rust-lang/rust#158838 (tidy: Use `empty_alternate = true` for triagebot mention glob check)

Check NotInModule early since it can be a lot faster

54cfcdf

rustbot assigned khyperia Jul 5, 2026

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 5, 2026

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 5, 2026

rust-bors Bot pushed a commit that referenced this pull request Jul 5, 2026

Auto merge of #158819 - steffahn:uninhab-module-check-first, r=<try>

f0c1ec5

Put `InhabitedPredicate::NotInModule` earlier in disjunction since it can be a lot faster

steffahn mentioned this pull request Jul 5, 2026

lemmy_db_views_modlog_combined has a weird performance regression #141006

Open

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 5, 2026

rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2026

jhpratt mentioned this pull request Jul 6, 2026

Rollup of 24 pull requests #158847

Merged

rust-bors Bot merged commit 8c8f363 into rust-lang:main Jul 6, 2026
14 checks passed

rustbot added this to the 1.99.0 milestone Jul 6, 2026

Uh oh!

Uh oh!

Conversation

steffahn commented Jul 5, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 5, 2026

Uh oh!

hanna-kruppe commented Jul 5, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

steffahn commented Jul 5, 2026

Uh oh!

rust-bors Bot commented Jul 5, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Jul 5, 2026

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

steffahn commented Jul 5, 2026

Footnotes

Uh oh!

khyperia commented Jul 6, 2026

Uh oh!

steffahn commented Jul 6, 2026

Uh oh!

khyperia commented Jul 6, 2026

Uh oh!

rust-bors Bot commented Jul 6, 2026

Uh oh!

panstromek commented Jul 6, 2026

Uh oh!

Uh oh!

steffahn commented Jul 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

steffahn commented Jul 5, 2026 •

edited by rustbot

Loading