Option::map_or is being instantiated too much

[dtolnay]

There should not be 59 copies of this.

$ cargo llvm-lines | head -10

   6085   59  <core::option::Option<T>>::map_or
   1754   74  core::ptr::drop_in_place
   1486   13  <core::result::Result<T, E>>::map
   1188    3  serde_json::read::SliceRead::parse_str_bytes
   1096    1  <&'a mut serde_json::de::Deserializer<R> as serde::de::Deserializer<'de>>::deserialize_any
    931   10  <core::option::Option<T>>::map
    875    8  <core::result::Result<T, E>>::map_err
    862    2  core::str::pattern::TwoWaySearcher::next
    644    2  serde_json::ser::format_escaped_str_contents
    640    2  serde_json::value::ser::<impl serde::ser::Serialize for serde_json::value::Value>::serialize

$ rg '\bmap_or\b'

src/value/de.rs
67:                Ok(Number::from_f64(value).map_or(Value::Null, Value::Number))

src/value/from.rs
68:        Number::from_f64(f).map_or(Value::Null, Value::Number)

src/value/partial_eq.rs
13:        self.as_str().map_or(false, |s| s == other)
19:        self.as_str().map_or(false, |s| s == *other)
25:        other.as_str().map_or(false, |s| s == self)
31:        other.as_str().map_or(false, |s| s == *self)
37:        self.as_str().map_or(false, |s| s == other)
44:        other.as_str().map_or(false, |s| s == self)
53:                    self.$conversion().map_or(false, |i| i == (*other as $base))
59:                    other.$conversion().map_or(false, |i| i == (*self as $base))
65:                    self.$conversion().map_or(false, |i| i == (*other as $base))
71:                    self.$conversion().map_or(false, |i| i == (*other as $base))

src/value/ser.rs
106:        Ok(Number::from_f64(value).map_or(Value::Null, Value::Number))

[arielb1]

I don't think map_or is instantiated too much - it needs to be instantiated at every use-site in order to work, because every use-site passes it a different closure.

However, the initial IR we emit for map_or is garbage quality - see rust-lang/rust#46758 - which causes the IR to bloat.

Note that map_or is this teeny little function, so there should be no reason for it to grow big:

    pub fn map_or<U, F: FnOnce(T) -> U>(self, default: U, f: F) -> U {
        match self {
            Some(t) => f(t),
            None => default,
        }
    }

[cramertj]

@arielb1

it needs to be instantiated at every use-site in order to work

Right-- but there are way more copies of map_or appearing than there are usages in the code. rust-lang/rust#46477 would account for this, as it's also generating a copy of each map_or for each generic instantiation of the surrounding function. That is, the following would generate six copies, not two:

fn foo<A>(_: A) {
    Some(()).map_or((), |()| println!("foo"));
}
fn foo2<A>(_: A) {
    Some(()).map_or((), |()| println!("foo"));
}

foo(());
foo(5);
foo("");
foo2(());
foo2(5);
foo2("");

[arielb1]

@cramertj

Sure in that case we could potentially merge the specializations, but

1. map_or is basically a tiny match - there's no good reason it should take an appreciable amount of compilation time (and if it does, then an open-coded match would probably take a similar amount too).
2. At least in the serde example, map_or is used in PartialEq impls and the closure within it does capture the type parameter, so merging won't help.

BTW:

json/src/value/partial_eq.rs

Lines 48 to 83 in b4e061f

	macro_rules! partialeq_numeric {
	($([$($ty:ty)*], $conversion:ident, $base:ty)*) => {
	$($(
	impl PartialEq<$ty> for Value {
	fn eq(&self, other: &$ty) -> bool {
	self.$conversion().map_or(false, |i| i == (*other as $base))
	}
	}
	impl PartialEq<Value> for $ty {
	fn eq(&self, other: &Value) -> bool {
	other.$conversion().map_or(false, |i| i == (*self as $base))
	}
	}
	impl<'a> PartialEq<$ty> for &'a Value {
	fn eq(&self, other: &$ty) -> bool {
	self.$conversion().map_or(false, |i| i == (*other as $base))
	}
	}
	impl<'a> PartialEq<$ty> for &'a mut Value {
	fn eq(&self, other: &$ty) -> bool {
	self.$conversion().map_or(false, |i| i == (*other as $base))
	}
	}
	)*)*
	}
	}
	partialeq_numeric! {
	[i8 i16 i32 i64 isize], as_i64, i64
	[u8 u16 u32 u64 usize], as_u64, u64
	[f32 f64], as_f64, f64
	[bool], as_bool, bool
	}

This is a macro - I see 13*4=52 distinct non-generic calls to map_or in that block, all unmergable because they have different types. Together with the 6 other closure calls, and the 2 identical calls to map_or(Value::Null, Value::Number), all monomorphizations are accounted for.

[arielb1]

So I don't think map_or is instantiated too much, but rather:

1. There are 58 different PartialEq impls - none of them generic - and all have to be codegen-ed
2. The codegen for map_or is terrible. I'm trying to figure out how to improve this.

[dtolnay]

Sorry the OP is worded in a confusing way. I did not dig into this much before filing the issue but from looking at it now, this is a serde_json bug not a rustc bug. It is computing:

// impl PartialEq<i8> for Value
self.as_i64().map_or(false, |i| i == (*other as i64))

// impl PartialEq<i16> for Value
self.as_i64().map_or(false, |i| i == (*other as i64))

// impl PartialEq<i32> for Value
self.as_i64().map_or(false, |i| i == (*other as i64))

// impl PartialEq<i64> for Value
self.as_i64().map_or(false, |i| i == (*other as i64))

// impl PartialEq<isize> for Value
self.as_i64().map_or(false, |i| i == (*other as i64))

// etc

We (in serde_json) should be able to rearrange this to instantiate map_or much less. Maybe not exactly the following code but conceptually:

let other = *other as i64;
self.as_i64().map_or(false, |i| i == other)

[dtolnay]

Until rust-lang/rust#46477 is addressed, we will need a helper function instead of calling map_or directly from each PartialEq impl.

eq_i64(self, *other as i64)

fn eq_i64(value: &Value, other: i64) {
    // instantiate map_or once
    value.as_i64().map_or(false, |i| i == other)
}

[arielb1]

And even with it addressed, each PartialEq impl in a macro will use a different closure, and therefore will be un-mergeable.

[dtolnay]

Hmm I don't think so @arielb1. We would use one helper function across Value == u64, u64 == Value, &Value == u64, and &mut Value == u64. We would use another one across Value == str, Value == &str, str == Value, &str == Value, Value == String, and String == Value. In the end I think there will be 6 instantiations of map_or (down from 59).

[arielb1]

@dtolnay

If you move everything to helper functions, then you won't need rust-lang/rust#46477

[dtolnay]

Oh I see what you mean. Yes that is correct. Thanks!