Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

timely-util/builder_async: offer automatically yielding give API #27866

Merged
merged 4 commits into from
Jun 26, 2024

Conversation

petrosagg
Copy link
Contributor

Motivation

This PR offers a fueled give API that automatically yields back to timely after certain number of bytes have been emitted into the dataflow. The value is currently capped at 128MB, which is the size of a persist part. This API is only available for operators that make use of StackWrapper containers which can accurately track their heap size.

As part of this PR I also made the original give and give_container methods synchronous. The only reason they were async in the first place was because I had initially envisioned this auto-yield API to be the default and so the async-ness was reserved. Since I no longer think this is a good idea I removed the async keyword.

Tips for reviewer

The diff is big due to the last commit that contains all the removed .await calls. Only the changes in timely-util need review. Everything else is mechanical.

Checklist

@petrosagg petrosagg requested review from aljoscha, danhhz, jkosh44 and a team as code owners June 25, 2024 13:38
@jkosh44 jkosh44 removed their request for review June 25, 2024 13:44
@petrosagg petrosagg force-pushed the fueled-give2 branch 2 times, most recently from a1f6b22 to d568e3d Compare June 25, 2024 14:29
Normal `Capability` values are the only thing currently exposed to users
so there is no use for that trait anymore.

Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
@petrosagg petrosagg requested review from antiguru and removed request for aljoscha, danhhz and a team June 26, 2024 11:36
They are now synchronous so this commit removes all the `.await`s.

Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
Copy link
Member

@antiguru antiguru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


/// Provides one record at the time specified by the capability. This method will automatically
/// yield back to timely after [Self::MAX_OUTSTANDING_BYTES] have been produced.
pub async fn give_fueled<D2>(&mut self, cap: &Capability<T>, data: D2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to future selves: Should this be #[inline]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this will always be considered for inlining (at least according to this article https://matklad.github.io/2021/07/09/inline-in-rust.html#Inlining-in-Rust) because the method is generic

@petrosagg petrosagg merged commit d1aa54d into MaterializeInc:main Jun 26, 2024
76 checks passed
@petrosagg petrosagg deleted the fueled-give2 branch June 26, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants