Skip to content

Comments

WIP: replaced Vec with HashSet in gstreamer muteables#278

Closed
khodzha wants to merge 1 commit intoservo:masterfrom
khodzha:hashset
Closed

WIP: replaced Vec with HashSet in gstreamer muteables#278
khodzha wants to merge 1 commit intoservo:masterfrom
khodzha:hashset

Conversation

@khodzha
Copy link
Contributor

@khodzha khodzha commented Jul 7, 2019

@ferjm you've been suggesting HashSet from the start yet because we didn't have an id back then I went with Vec.

But since we already add usize ids to muteables Vec I suppose we can just utilize it for Hash + Eq and replace Vec with HashSet now.

(I'm not sure whether IdMuteable is appropriate naming at all and also whether it should go straight to lib.rs file or be separated into another one)

Copy link
Contributor

@ferjm ferjm left a comment

Choose a reason for hiding this comment

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

Thanks, @khodzha! A HashSet is an improvement, but I think we can do it without duplicating the ids.

vec.retain(|m| m.0 != muteable_id);
if vec.len() == 0 {
if let Some(set) = muteables.get_mut(&id) {
set.retain(|m| m.id != muteable_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

The benefit of using a HashSet here is to avoid this loop, right? So this should probably be:

if !set.remove(mutable_id) {
    warn!("Tried to remove an unknown Muteable");
}

if vec.len() == 0 {
if let Some(set) = muteables.get_mut(&id) {
set.retain(|m| m.id != muteable_id);
if set.len() == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

is_empty is more idiomatic.

}

struct IdMuteable {
id: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

Players and AudioContexts already have their own ids. I think there is no need to duplicated them here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but we have only Weak<Mutex<dyn Muteable>>, is locking mutex just to get an id fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think that should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, this id is also used for Eq + Hash!

static ref BACKEND_BASE_TIME: gst::ClockTime = { gst::SystemClock::obtain().get_time() };
}

struct IdMuteable {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Let's rename this to MuteableRef.

@khodzha
Copy link
Contributor Author

khodzha commented Jul 10, 2019

After a discussion with @ferjm it was decided HashSet doesnt provide much of a benefit since in current implementation retain() loop cant be replaced with a remove() (which was the idea behind using HashSet in the first place).

So I'm closing this PR.

@khodzha khodzha closed this Jul 10, 2019
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