search

mum-rs / mum

Daemon/cli mumble client
MIT License
31 stars 2 forks source link

Rewrite mumd to allow TCP/UDP/Audio parallelist #91

Closed rbran closed 3 years ago

rbran commented 3 years ago

First of all: Thanks for the software, I was able to learn a lot about async rust.

But mainly sorry for the state of this PR. I tried to implemente parallelism in TCP/UDP/Audio/etc, but ended up almost completely rewriting mumd, it obviously got out-of-hand.

Although it seems functional, there still a lot to be done, as some functionalities are left unimplemented. I may finish this in the future.

In any case, this solves my use-case. I'm just sharing in case you guys want to take a look.

default-username-852 commented 3 years ago

First of all, thanks for the PR! I haven't really looked at the code (in part due to the big diff), but I don't really get what issue this PR solves. I think the way that our networking code is implemented is pretty parallel as it is. Feel free to clarify what issue the PR solves and how!

Also, this PR introduces some regressions with regards to certain features. For example, mumctl status doesn't work and there aren't any notification sounds. There also seem to be some compiler warnings.

I'm not against merging this PR, and if you want it merged, I'll happily review the code, but I don't think it is ready to be merged as it stands right now.

rbran commented 3 years ago

Sorry, I closed by mistake

rbran commented 3 years ago

I don't really get what issue this PR solves. I think the way that our networking code is implemented is pretty parallel as it is.

Correct if I'm wrong, but the only way to have parallel execution is tokio is using the tokio::spawn (or other spawn like) function.

This is mentioned on select page:

By running all async expressions on the current task, the expressions are able to run concurrently but not in parallel.

This PR does mostly reorganize the code, so the tasks "decouple" from one another, and can run in parallel more efficiently, this involves using lots of channels and RwLock to avoid deadlocks.

I think (correct me if I'm wrong) the mumd process could only run ~4 "parts" in parallel:

Also, this PR introduces some regressions with regards to certain features. For example, mumctl status doesn't work and there aren't any notification sounds. There also seem to be some compiler warnings.

Yes, I agree, this need some work. Unfortunately I run out of free time before I could finish this, I may have more time in the near future.

I'm not against merging this PR, and if you want it merged, I'll happily review the code, but I don't think it is ready to be merged as it stands right now.

I think is better not to merge this, at least not at this stage.

Fell free to close this PR, I may finish this in the future and make a new one, although I can't garante that. But, if you take a look and give some feedback before closing, I could add those to the next interaction.

default-username-852 commented 3 years ago

You are right in that there are basically only a couple of true parallel tasks running at the same time. I don't think that is a problem though. I profiled mumd during normal usage and the futures spent something like 99% of the time waiting. Also, I think there are easier ways of achieving more parallelism if that is desired (for example spawning the futures and selecting on the JoinHandles instead of just selecting on the futures). I therefore don't think it is worth putting more effort into this PR, since the gains are small, and the work to identify and fix all regressions is probably pretty big.