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

Stream/General improvements #1041

Merged
merged 6 commits into from
Sep 9, 2024
Merged

Conversation

insipx
Copy link
Contributor

@insipx insipx commented Sep 6, 2024

Non-wasm specific improvements pulled from work from wasm:

  • improve streams. Since Rust 1.75 async fn in trait is supported, along with a much lighter replacement to async_trait.

    • This allows us to make our native streams completely stack-based, removing Pin<Box<dyn Stream>>
    • removes async-trait from everywhere except a trait in xmtp_id which requires dynamic dispatch
    • removing async-trait should also generally improve compile times
    • more flexible stream traits with associated types
    • using stack-based streams, means we can get rid of lots of clones() and use references instead
    • sets us up nicely for these new upcoming rust 2024 edition trait rules
  • removes some unnecessary dependencies

    • flume, used in one place
    • smart-default used in once place
  • removes an expensive clone we were doing in stream_all_messages preferring an Arc<> clone instead

  • replaces ethers_core with ethers::core

  • removes a random unsafe code block in xmtp_id

  • unifies trait InboxOwner (uses one in xmtp_id, deletes elsewhere)

@insipx insipx requested a review from a team as a code owner September 6, 2024 23:11
@insipx insipx force-pushed the insipx/pull-improvements-from-wasm branch from 914f2d9 to beebf2f Compare September 6, 2024 23:12
let client = self.clone();

let group_id_to_info = group_id_to_info.clone();
let group_info = group_id_to_info.clone();
Copy link
Contributor Author

@insipx insipx Sep 6, 2024

Choose a reason for hiding this comment

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

Previously, this would clone the entire HashMap<> (list of groups) for every single group message, where the key is a Vec<u8>. now we just clone the Arc.

@insipx insipx enabled auto-merge (squash) September 6, 2024 23:40
@insipx insipx disabled auto-merge September 7, 2024 02:30
pub async fn stream<'a, ApiClient>(
&'a self,
client: &'a Client<ApiClient>,
) -> Result<impl Stream<Item = StoredGroupMessage> + '_, GroupError>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This lifetime capture (+ '_) becomes the default behavior in rust 2024 edition + allows us to attach more flexible lifetime captures wherever we use impl Trait, which will allow eliding the 'alifetime here

Copy link
Contributor

@nplasterer nplasterer left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@neekolas neekolas left a comment

Choose a reason for hiding this comment

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

This is a huge set of improvements

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.

3 participants