optimize `superset` method of `IntervalSet` #97862

SparrowLii · 2022-06-08T07:30:00Z

Given that intervals in the IntervalSet are sorted and strictly separated( it means the end of the previous interval will not be equal to the start of the next interval), we can reduce the complexity of the superset method from O(NMlogN) to O(2N) (N is the number of intervals and M is the length of each interval)

rust-highfive · 2022-06-08T07:30:04Z

r? @lcnr

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

lcnr · 2022-06-08T07:52:01Z

impl looks good

While you're at it, can you document the invariants of IntervalSet? Your impl requires that adjacent intervals are merged. That is okay as e.g. [1..=4, 5..=7] is currently not a possible IntervalSet.

I would like to have that documented, or even better, I would like a method fn check_invariants which just asserts that and is used after mutating operations if debug_assertions are enabled.

@bors try @rust-timer queue

rust-timer · 2022-06-08T07:52:02Z

Awaiting bors try build completion.

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

bors · 2022-06-08T07:52:12Z

⌛ Trying commit 8db6d4b with merge b0e738d5614446ebae0880e368e25c36c57f258c...

bors · 2022-06-08T09:49:03Z

☀️ Try build successful - checks-actions
Build commit: b0e738d5614446ebae0880e368e25c36c57f258c (b0e738d5614446ebae0880e368e25c36c57f258c)

rust-timer · 2022-06-08T09:49:05Z

Queued b0e738d5614446ebae0880e368e25c36c57f258c with parent 47aee31, future comparison URL.

rust-timer · 2022-06-08T11:07:11Z

Finished benchmarking commit (b0e738d5614446ebae0880e368e25c36c57f258c): comparison url.

Instruction count

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

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	0.6%	0.6%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	0.6%	0.6%	1

Cycles

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-3.3%	-3.3%	1
Improvements 🎉 (secondary)	-2.2%	-2.2%	1
All 😿🎉 (primary)	-3.3%	-3.3%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

SparrowLii · 2022-06-08T14:27:00Z

Thanks for reviewing! I added the check_invariants method and fixed an impl error in the insert_all method

lcnr · 2022-06-08T14:32:01Z

nice to see that checking the invariants actually found a bug 😆 even if that bug probably doesn't result in bad behavior as it only affects empty domains.

@bors r+

bors · 2022-06-08T14:32:02Z

📌 Commit 65a5b08 has been approved by lcnr

lcnr · 2022-06-08T14:44:11Z

compiler/rustc_index/src/interval.rs

+            }
+            current = Some(*end);
+        }
+        current.map_or(true, |x| x < self.domain as u32)


it seems like that invariant isn't actually assumed rn? 🤔 generally, the domain seems mostly irrelevant, could "just" use u32::MAX instead for the range end.

🤷 don't really care how you deal with the test failure

I think we'd better make sure all intervals don't exceed domain, after all, both insert_all and insert_range use domain as the range maximum. I increased the domain in the test from 3000 to 10000, which I think should be reasonable

lcnr · 2022-06-08T15:05:38Z

@bors r+

bors · 2022-06-08T15:05:40Z

📌 Commit 726b35b has been approved by lcnr

bors · 2022-06-09T07:13:49Z

⌛ Testing commit 726b35b with merge 6dc598a...

bors · 2022-06-09T09:56:24Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 6dc598a to master...

rust-timer · 2022-06-09T11:14:32Z

Finished benchmarking commit (6dc598a): comparison url.

Instruction count

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

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.1%	-2.5%	3
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.6%	-3.0%	2
Improvements 🎉 (secondary)	-3.9%	-3.9%	1
All 😿🎉 (primary)	-2.6%	-3.0%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

optimize superset method of IntervalSet

8db6d4b

rust-highfive assigned lcnr Jun 8, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 8, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 8, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 8, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 8, 2022