.*: Box all Futures that are larger than 8KiB #23283

ParkMyCar · 2023-11-17T18:24:42Z

This PR boxes (aka moves to the heap) all Rust Futures that are larger than 8KiB, and enables the large_futures clippy lint.

Motivation

Calling Rust Futures currently can result in inefficient code with excessive memcpy's, as documented by this issue: rust-lang/rust#99504. There are a number of Futures in our codebase which are quite large >8KiB, and thus could see poor performance. By Box-ing the Futures we'd end up memcpy-ing only a fat pointer, which is 16 bytes, which should be a good performance improvement.

Discussed this in Slack

Tips for reviewer

This PR is split into two commits that can be reviewed separately:

Enabling the new large_futures lint.
Boxing all of the Futures.

When boxing a Future there are two possible approaches, either changing the async fn to fn -> BoxFuture or using .boxed() at the callsite. I generally used .boxed() and only when a function was called multiple times, didn't use generics, and it didn't complicate the return type too much, would I update from async fn to fn -> BoxFuture.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:

def-

I triggered nightly and coverage would appreciate waiting for the results.

(I didn't expect the Rust issue to have been opened by a former SAP colleague of mine, hi Ivan!)

jkosh44

Surfaces changes LGTM. I personally don't understand the difference between boxed() and boxed_local(), it might be useful to add that to the PR description.

I kicked off a scalability benchmark: https://buildkite.com/materialize/nightlies/builds/5283, I thought the results might be interesting.

jkosh44 · 2023-11-17T18:29:16Z

misc/python/materialize/cli/gen-lints.py

+    # Futures get compiled into state machines, which can end up being very large (10's of KB).
+    # It's inefficient to moves these around on the stack, so require that they get boxed.


It might be useful to include a link to rust-lang/rust#99504 here.

jkosh44 · 2023-11-17T18:37:26Z

src/environmentd/src/http.rs

@@ -529,36 +529,40 @@ pub struct AuthedClient {
 }

 impl AuthedClient {
-    async fn new(
-        adapter_client: &Client,
+    /// Note: the returned Future is intentionally boxed because it is very large.


This is subjective, so it's ultimately up to you, but I think these comments are a little redundant. As long as the lint has a sufficient explanation somewhere around it, I think we can remove these comments.

+1! I also find these comments more noisy than helpful

I agree! I assume people that wonder about this would try to remove the boxed call and find out the reason for it through the lint.

bkirwi · 2023-11-17T18:51:03Z

src/persist-client/src/read.rs

@@ -689,17 +690,30 @@ where
    /// The `Since` error indicates that the requested `as_of` cannot be served
    /// (the caller has out of date information) and includes the smallest
    /// `as_of` that would have been accepted.
-    #[instrument(level = "debug", skip_all, fields(shard = %self.machine.shard_id()))]
-    pub async fn listen(self, as_of: Antichain<T>) -> Result<Listen<K, V, T, D>, Since<T>> {


I'm a bit sad that we're changing signatures here... in particular it seems brittle if this method is refactored.

IIUC it would be ~nearly as good to insert a .boxed() before the .await and leave the signature alone... is that right, and if so how would you feel about it?

We definitely can revert the change to the signature!

The only reason I originally made the changes instead of using .boxed() was because certain functions were called numerous times, and it seemed easier to change the signature rather than all of the callsites.

Yeah +1 on this too, I dislike baking this into signatures. As long as the lint catches it if the boxed call is missed at a callsite, then I have a decently strong preference to do that everywhere in the persist bits of this change.

ParkMyCar · 2023-11-17T19:24:39Z

Moving this to the draft stage while I work out if it actually impacts performance or not, given the size of the PR!

ParkMyCar · 2023-11-17T19:29:41Z

I kicked off a scalability benchmark: https://buildkite.com/materialize/nightlies/builds/5283, I thought the results might be interesting.

Thanks for kicking off the scalability benchmark! If anything were to show an improvement I think that would be the one

danhhz

Neat find!

danhhz · 2023-11-17T19:59:07Z

src/environmentd/src/http.rs

@@ -529,36 +529,40 @@ pub struct AuthedClient {
 }

 impl AuthedClient {
-    async fn new(
-        adapter_client: &Client,
+    /// Note: the returned Future is intentionally boxed because it is very large.


+1! I also find these comments more noisy than helpful

danhhz · 2023-11-17T20:02:26Z

src/persist-client/src/read.rs

@@ -689,17 +690,30 @@ where
    /// The `Since` error indicates that the requested `as_of` cannot be served
    /// (the caller has out of date information) and includes the smallest
    /// `as_of` that would have been accepted.
-    #[instrument(level = "debug", skip_all, fields(shard = %self.machine.shard_id()))]
-    pub async fn listen(self, as_of: Antichain<T>) -> Result<Listen<K, V, T, D>, Since<T>> {


Yeah +1 on this too, I dislike baking this into signatures. As long as the lint catches it if the boxed call is missed at a callsite, then I have a decently strong preference to do that everywhere in the persist bits of this change.

teskje

Compute changes LGTM.

teskje · 2023-11-17T20:00:02Z

src/adapter-types/src/lib.rs

@@ -73,6 +73,7 @@
 #![warn(clippy::disallowed_macros)]
 #![warn(clippy::disallowed_types)]
 #![warn(clippy::from_over_into)]
+#![warn(clippy::large_futures)]


Very exited about moving these to the workspace Cargo.toml and getting out of the business of having to touch every crate when we adjust a lint :)

teskje · 2023-11-17T20:03:16Z

src/environmentd/src/http.rs

@@ -529,36 +529,40 @@ pub struct AuthedClient {
 }

 impl AuthedClient {
-    async fn new(
-        adapter_client: &Client,
+    /// Note: the returned Future is intentionally boxed because it is very large.


I agree! I assume people that wonder about this would try to remove the boxed call and find out the reason for it through the lint.

guswynn

storage stuff looks good!

@jkosh44 boxed_local just doesn't require the boxed future is Send!

guswynn · 2023-11-20T17:26:32Z

src/storage-controller/src/rehydration.rs

-                Some(RehydrationCommand::Send(command)) => {
-                    self.absorb_command(&command);
+    /// Note: the returned Future is intentionally boxed because it is very large.
+    fn step_await_address<'a>(&'a mut self) -> BoxFuture<'a, RehydrationTaskState<T>> {


im surprised this one is so big!

petrosagg · 2023-11-20T18:38:00Z

I tried to interpret the scalability benchmark results and this didn't seem to move the needle much. I'm starting to lean against merging this lint as it is applying a premature optimization to many places that don't really benefit by it. An analogy I thought is that we also don't always bother to reuse a vector or use with_capacity in functions that are rarely called because that improves readability and the extra ceremony to reuse the allocation would be a net negative. I believe we would also be against a lint that forced you to be careful with allocations and we would rather only do it when it makes sense.

So all in all I think we should punt on this and box futures if we discover that we spend a lot of time copying memory in some critical part of the code.

ParkMyCar · 2024-01-08T15:08:53Z

Closing this because we're currently unsure of the actual impact

ParkMyCar requested a review from benesch as a code owner November 17, 2023 18:24

ParkMyCar requested a review from a team November 17, 2023 18:24

ParkMyCar requested a review from a team as a code owner November 17, 2023 18:24

ParkMyCar requested a review from a team November 17, 2023 18:24

ParkMyCar requested a review from a team as a code owner November 17, 2023 18:24

ParkMyCar requested a review from a team November 17, 2023 18:24

ParkMyCar requested review from a team, aalexandrov and umanwizard as code owners November 17, 2023 18:24

ParkMyCar requested review from jkosh44 and petrosagg November 17, 2023 18:24

start, update lints

6df554b

def- requested review from def- and removed request for a team November 17, 2023 18:29

ParkMyCar force-pushed the lint/large-futures branch from 2ed1871 to 2b32857 Compare November 17, 2023 18:30

def- reviewed Nov 17, 2023

View reviewed changes

jkosh44 approved these changes Nov 17, 2023

View reviewed changes

box all futures that are larger than 8KiB

c6f9ac9

ParkMyCar force-pushed the lint/large-futures branch from 2b32857 to c6f9ac9 Compare November 17, 2023 18:42

bkirwi reviewed Nov 17, 2023

View reviewed changes

ParkMyCar marked this pull request as draft November 17, 2023 19:24

danhhz reviewed Nov 17, 2023

View reviewed changes

teskje approved these changes Nov 17, 2023

View reviewed changes

guswynn approved these changes Nov 20, 2023

View reviewed changes

ParkMyCar closed this Jan 8, 2024

ParkMyCar deleted the lint/large-futures branch June 17, 2024 20:37

		# Futures get compiled into state machines, which can end up being very large (10's of KB).
		# It's inefficient to moves these around on the stack, so require that they get boxed.

.*: Box all Futures that are larger than 8KiB #23283

.*: Box all Futures that are larger than 8KiB #23283

Uh oh!

Conversation

ParkMyCar commented Nov 17, 2023

Motivation

Tips for reviewer

Checklist

Uh oh!

def- left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkosh44 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ParkMyCar commented Nov 17, 2023

Uh oh!

ParkMyCar commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danhhz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teskje left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guswynn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrosagg commented Nov 20, 2023

Uh oh!

ParkMyCar commented Jan 8, 2024

Uh oh!

Uh oh!

def- left a comment •

edited

Loading

ParkMyCar commented Nov 17, 2023 •

edited

Loading