DebugBinds/Query: Reduce monomorphization overhead

[Turbo87]

The output of cargo llvm-lines --package diesel_tests --test integration_tests --profile test --features postgres showed the Debug and Display implementations of the DebugBinds and DebugQuery structs as two of the main code size reasons, slowing down the compilation of applications. This commit significantly reduces the LLVM IR lines of the two structs, which should hopefully improve compile times a little bit (see commit messages for detailed numbers).

/cc @weiznich

Related:

• https://matklad.github.io/2021/09/04/fast-rust-builds.html#Keeping-an-Eye-on-Instantiations

[Turbo87]

I tried to use a tri!() macro instead (see https://github.com/serde-rs/serde/blob/v1.0.217/serde/src/lib.rs#L290-L301), but that slightly bumped the numbers upwards again, so I'm not sure whether it's worth it for the readability benefit.

[weiznich]

Thanks for opening this PR. Such optimizations are really appreciated. I wonder whether it might be meaningful to use a similar strategy here to what we already do for the statement cache:

diesel/diesel/src/connection/statement_cache/mod.rs

Lines 252 to 287 in 3f69aa4

	/// Implemented for all `QueryFragment`s, dedicated to dynamic dispatch within the context of
	/// `statement_cache`
	///
	/// We want the generated code to be as small as possible, so for each query passed to
	/// [`StatementCache::cached_statement`] the generated assembly will just call a non generic
	/// version with dynamic dispatch pointing to the VTABLE of this minimal trait
	///
	/// This preserves the opportunity for the compiler to entirely optimize the `construct_sql`
	/// function as a function that simply returns a constant `String`.
	#[allow(unreachable_pub)]
	#[cfg_attr(
	docsrs,
	doc(cfg(feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"))
	)]
	pub trait QueryFragmentForCachedStatement<DB> {
	/// Convert the query fragment into a SQL string for the given backend
	fn construct_sql(&self, backend: &DB) -> QueryResult<String>;
	/// Check whether it's safe to cache the query
	fn is_safe_to_cache_prepared(&self, backend: &DB) -> QueryResult<bool>;
	}
	impl<T, DB> QueryFragmentForCachedStatement<DB> for T
	where
	DB: Backend,
	DB::QueryBuilder: Default,
	T: QueryFragment<DB>,
	{
	fn construct_sql(&self, backend: &DB) -> QueryResult<String> {
	let mut query_builder = DB::QueryBuilder::default();
	self.to_sql(&mut query_builder, backend)?;
	Ok(query_builder.finish())
	}
	fn is_safe_to_cache_prepared(&self, backend: &DB) -> QueryResult<bool> {
	<T as QueryFragment<DB>>::is_safe_to_cache_prepared(self, backend)
	}
	}

That would mean that instead of storing a reference to the generic query type we would rather just store a reference to a trait object in DebugQuery and DebugBinds, which should hopefully allow to minimize the number of generated lines of code even further. If you have time and motivation to try that this would be really appreciated, otherwise it shouldn't be a problem to merge this PR as it is.

[Turbo87]

instead of storing a reference to the generic query type we would rather just store a reference to a trait object in DebugQuery and DebugBinds

yeah, I thought about that as well, but I wasn't sure whether the performance overhead of the Box would be acceptable (or how to measure it... 😅)

[Turbo87]

hmm, in general this seems to work, but https://github.com/diesel-rs/diesel/blob/3f69aa4570c505a94889e3c3de4e83e13bc6a37d/diesel/src/query_builder/insert_statement/insert_with_default_for_sqlite.rs isn't particularly happy about the removal of the generic T parameter. any advice on how to deal with this special case?

[weiznich]

Thanks for the update. I think removing the generic T type is unfortunately not possible, as that would be a breaking change. What we can do is to just put that type into a PhantomData and then have all the methods on that generic type call some static method without the generic parameter T internally. That should reduce the amount of duplication to the minimum while keeping things stable to the outside.

For a future diesel 3.0 version we might want to remove the T type there, but that's something for the future.,

[Turbo87]

just put that type into a PhantomData

that didn't work since the code in https://github.com/diesel-rs/diesel/blob/3f69aa4570c505a94889e3c3de4e83e13bc6a37d/diesel/src/query_builder/insert_statement/insert_with_default_for_sqlite.rs is accessing self.query.foo directly, but I've found another way to shrink it down :)

[Turbo87]

total LLVM IR lines as reported by the above command:

• before: 2_024_074
• after: 1_740_806 (-14%)

[weiznich]

Thanks for working on this and thanks for providing these numbers ❤️

[Turbo87]

/usr/bin/ld: cannot find -lsqlite3: No such file or directory

that seems unrelated to my changes 😅

[weiznich]

/usr/bin/ld: cannot find -lsqlite3: No such file or directory

that seems unrelated to my changes 😅

Yes, seems like the CI image got updated and something is now missing. I need to fix that 😞