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.

Sign up for GitHub

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

Jump to bottom

perf: Pack the state and future of unfolds in the same memory #2283

Merged
merged 3 commits into from
Dec 26, 2020
Merged

perf: Pack the state and future of unfolds in the same memory #2283

merged 3 commits into from
Dec 26, 2020

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Dec 8, 2020

taiki-e
taiki-e reviewed Dec 8, 2020
View reviewed changes
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me aside from a few nits.

futures-util/src/sink/unfold.rs Outdated Show resolved Hide resolved
futures-util/src/stream/unfold.rs Outdated Show resolved Hide resolved
futures-util/src/stream/unfold.rs Outdated Show resolved Hide resolved
@taiki-e taiki-e added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author label Dec 8, 2020
@Marwes
Copy link
Contributor Author

Marwes commented Dec 9, 2020

Had to updated this to manual pin projections due to #2273 .

@taiki-e taiki-e self-requested a review December 13, 2020 15:35
@taiki-e taiki-e removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author label Dec 13, 2020
taiki-e
taiki-e requested changes Dec 13, 2020
View reviewed changes
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks. The implementation LGTM, but seems CI is still failed. Could you fix it? (probably you need to replace std with core in unfold_state.rs)

In addition, there are many irrelevant format changes in lib.rs, could you remove them?

@taiki-e taiki-e added futures-util S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author A-sink Area: futures::sink A-stream Area: futures::stream and removed futures-util labels Dec 13, 2020
taiki-e
taiki-e reviewed Dec 25, 2020
View reviewed changes
futures-util/src/unfold_state.rs Outdated Show resolved Hide resolved
futures-util/src/unfold_state.rs Outdated Show resolved Hide resolved
futures-util/src/unfold_state.rs Outdated Show resolved Hide resolved
@taiki-e taiki-e merged commit 7ce5ce8 into rust-lang:master Dec 26, 2020
@Marwes Marwes mentioned this pull request Jan 7, 2021
Merged
@Marwes Marwes deleted the shrink_unfold branch January 12, 2021 16:31
@taiki-e taiki-e removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author label Jan 17, 2021
@taiki-e taiki-e mentioned this pull request Feb 13, 2021
Closed
exrook pushed a commit to exrook/futures-rs that referenced this pull request Apr 5, 2021
@Marwes
perf: Pack the state and future of unfolds in the same memory (rust-l…
92cdef9 
…ang#2283)

* Pack the state and future of unfolds in the same memory

* Use the same type for both sink and stream unfolds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piegamesde piegamesde piegamesde left review comments

@taiki-e taiki-e taiki-e approved these changes

Assignees
No one assigned
Labels
A-sink Area: futures::sink A-stream Area: futures::stream
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Marwes @piegamesde @taiki-e