Skip to content

Commit 646a3f8

Browse files
committed
Auto 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 eca9d93 + 907f5c1 commit 646a3f8

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
@@ -2444,7 +2444,7 @@ impl<K, V> Default for BTreeMap<K, V> {
24442444
#[stable(feature = "rust1", since = "1.0.0")]
24452445
impl<K: PartialEq, V: PartialEq, A: Allocator + Clone> PartialEq for BTreeMap<K, V, A> {
24462446
fn eq(&self, other: &BTreeMap<K, V, A>) -> bool {
2447-
self.iter().eq(other)
2447+
self.len() == other.len() && self.iter().zip(other).all(|(a, b)| a == b)
24482448
}
24492449
}
24502450

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)