Skip to content

Commit 4b4c761

Browse files
authored
Rollup merge of #149125 - zachs18:btreemap-eq-perf, r=workingjubilee
In `BTreeMap::eq`, do not compare the elements if the sizes are different. Reverts #147101 in library/alloc/src/btree/ #147101 replaced some instances of code like `a.len() == b.len() && a.iter().eq(&b)` with just `a.iter().eq(&b)`, but the optimization that PR introduced only applies for `TrustedLen` iterators, and `BTreeMap`'s itertors are not `TrustedLen`, so this theoretically regressed perf for comparing large `BTreeMap`/`BTreeSet`s with unequal lengths but equal prefixes, (and also made it so that comparing two different-length `BTreeMap`/`BTreeSet`s with elements whose `PartialEq` impls that can panic now can panic, though this is not a "promised" behaviour either way (cc #149122)) Given that `TrustedLen` is an unsafe trait, I opted to not implement it for `BTreeMap`'s iterators, and instead just revert the change. If someone else wants to audit `BTreeMap`'s iterators to make sure they always return the right number of items (even in the face of incorrect user `Ord` impls) and then implement `TrustedLen` for them so that the optimization works for them, then this can be closed in favor of that (or if the perf regression is deemed too theoretical, this can be closed outright). Example of theoretical perf regression: https://play.rust-lang.org/?version=beta&mode=release&edition=2024&gist=a37e3d61e6bf02669b251315c9a44fe2 (very rough estimates, using `Instant::elapsed`). In release mode on stable the comparison takes ~23.68µs. In release mode on beta/nightly the comparison takes ~48.351057ms.
2 parents 0b2d422 + 907f5c1 commit 4b4c761

File tree

3 files changed

+98
-1
lines changed

3 files changed

+98
-1
lines changed

library/alloc/src/collections/btree/map.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2416,7 +2416,7 @@ impl<K, V> Default for BTreeMap<K, V> {
24162416
#[stable(feature = "rust1", since = "1.0.0")]
24172417
impl<K: PartialEq, V: PartialEq, A: Allocator + Clone> PartialEq for BTreeMap<K, V, A> {
24182418
fn eq(&self, other: &BTreeMap<K, V, A>) -> bool {
2419-
self.iter().eq(other)
2419+
self.len() == other.len() && self.iter().zip(other).all(|(a, b)| a == b)
24202420
}
24212421
}
24222422

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
//! Regression tests which fail if some collections' `PartialEq::eq` impls compare
2+
//! elements when the collections have different sizes.
3+
//! This behavior is not guaranteed either way, so regressing these tests is fine
4+
//! if it is done on purpose.
5+
use std::cmp::Ordering;
6+
use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet, LinkedList};
7+
8+
/// This intentionally has a panicking `PartialEq` impl, to test that various
9+
/// collections' `PartialEq` impls don't actually compare elements if their sizes
10+
/// are unequal.
11+
///
12+
/// This is not advisable in normal code.
13+
#[derive(Debug, Clone, Copy, Hash)]
14+
struct Evil;
15+
16+
impl PartialEq for Evil {
17+
fn eq(&self, _: &Self) -> bool {
18+
panic!("Evil::eq is evil");
19+
}
20+
}
21+
impl Eq for Evil {}
22+
23+
impl PartialOrd for Evil {
24+
fn partial_cmp(&self, _: &Self) -> Option<Ordering> {
25+
Some(Ordering::Equal)
26+
}
27+
}
28+
29+
impl Ord for Evil {
30+
fn cmp(&self, _: &Self) -> Ordering {
31+
// Constructing a `BTreeSet`/`BTreeMap` uses `cmp` on the elements,
32+
// but comparing it with with `==` uses `eq` on the elements,
33+
// so Evil::cmp doesn't need to be evil.
34+
Ordering::Equal
35+
}
36+
}
37+
38+
// check Evil works
39+
#[test]
40+
#[should_panic = "Evil::eq is evil"]
41+
fn evil_eq_works() {
42+
let v1 = vec![Evil];
43+
let v2 = vec![Evil];
44+
45+
_ = v1 == v2;
46+
}
47+
48+
// check various containers don't compare if their sizes are different
49+
50+
#[test]
51+
fn vec_evil_eq() {
52+
let v1 = vec![Evil];
53+
let v2 = vec![Evil; 2];
54+
55+
assert_eq!(false, v1 == v2);
56+
}
57+
58+
#[test]
59+
fn hashset_evil_eq() {
60+
let s1 = HashSet::from([(0, Evil)]);
61+
let s2 = HashSet::from([(0, Evil), (1, Evil)]);
62+
63+
assert_eq!(false, s1 == s2);
64+
}
65+
66+
#[test]
67+
fn hashmap_evil_eq() {
68+
let m1 = HashMap::from([(0, Evil)]);
69+
let m2 = HashMap::from([(0, Evil), (1, Evil)]);
70+
71+
assert_eq!(false, m1 == m2);
72+
}
73+
74+
#[test]
75+
fn btreeset_evil_eq() {
76+
let s1 = BTreeSet::from([(0, Evil)]);
77+
let s2 = BTreeSet::from([(0, Evil), (1, Evil)]);
78+
79+
assert_eq!(false, s1 == s2);
80+
}
81+
82+
#[test]
83+
fn btreemap_evil_eq() {
84+
let m1 = BTreeMap::from([(0, Evil)]);
85+
let m2 = BTreeMap::from([(0, Evil), (1, Evil)]);
86+
87+
assert_eq!(false, m1 == m2);
88+
}
89+
90+
#[test]
91+
fn linkedlist_evil_eq() {
92+
let m1 = LinkedList::from([Evil]);
93+
let m2 = LinkedList::from([Evil; 2]);
94+
95+
assert_eq!(false, m1 == m2);
96+
}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
mod binary_heap;
2+
mod eq_diff_len;

0 commit comments

Comments
 (0)