Skip to content
This repository has been archived by the owner on Oct 15, 2022. It is now read-only.

Upgrade notify, switch to crossbeam_channel #217

Merged
merged 2 commits into from
Nov 14, 2019

Conversation

curiousleo
Copy link
Collaborator

@curiousleo curiousleo commented Nov 13, 2019

Prerequisite for a PR to simplify the build loop. This PR contains zero logic changes, it's purely a search-and-replace affair. I've split it out because I think it should be quick and easy to review.

What's in this PR?

  • Upgrade notify crate from v4 to v5
  • Switch from mpsc channels to crossbeam_channel (used by the new version of notify)

Why upgrade notify?
Notify v4 has been abandoned by its author. Notify v5 is what the new maintainers are working on now. It's still in pre-release, but AFAICT that just means the API hasn't been 100% stabilised yet. So we pin the exact version to avoid surprises.

Why switch to crossbeam_channel?
crossbeam_channel is a more performant drop-in replacement for mpsc with more features.

@@ -9,24 +9,25 @@ license = "Apache-2.0"
edition = "2018"

[dependencies]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't resist sorting the dependencies alphabetically.

Copy link
Collaborator Author

@curiousleo curiousleo Nov 13, 2019

Choose a reason for hiding this comment

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

The only changes here are:

  • changed notify = "4.0.6" to notify = "5.0.0-pre.1"
  • added crossbeam-channel = "0.3.8"

@curiousleo curiousleo marked this pull request as ready for review November 13, 2019 15:26
@curiousleo
Copy link
Collaborator Author

PTAL

@Profpatsch
Copy link
Collaborator

Profpatsch commented Nov 14, 2019

It's still in pre-release, but AFAICT that just means the API hasn't been 100% stabilised yet. So we pin the exact version to avoid surprises.

Not entirely sure we want to update to a 5.0 pre-release yet, especially since v4 is still getting bug fix updates.

The ability to select, i.e. to wait on multiple channels at once, is IMO a crucial part of any channel API.

Agreed.

It sounds like you need the ability to select on channels for the build_loop update? If so, then LGTM.

@curiousleo
Copy link
Collaborator Author

It sounds like you need the ability to select on channels for the build_loop update? If so, then LGTM.

Yeah, the idea is for the build loop to become essentially

loop {
    select! {
        // receive simultaneously from various channels that might trigger a rebuild
    }
}

@Profpatsch
Copy link
Collaborator

Yeah, the idea is for the build loop to become essentially

loop {
    select! {
        // receive simultaneously from various channels that might trigger a rebuild
    }
}

Good thinking, that’s my biggest issue with #214 as it stands, that build_loop is becoming too complicated.

@Profpatsch Profpatsch merged commit f29d924 into target:master Nov 14, 2019
@curiousleo curiousleo mentioned this pull request Jan 9, 2020
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants