Make ads::client::Client sync

[cartercanedy]

This is the minimum diff required to make ads::client::Client sync. I considered making a special Error variant specific to locks, but I'm pretty sure that there's no way application code to induce a poisoned lock, and any poisons that come from standard library panics should definitely result in an aborted application.

Feel free to modify.

• Closes Making ads::client::Client implement the Send and Sync traits by not using RefCell and Cell in it #12

[birkenfeld]

Hi, thanks for the PR!

The problem is that when communicate can be called concurrently, replies are not guaranteed to end up where they need to go - if two threads start a request at the same time, the reader thread will just send both replies to the channel, and each calling thread will get one, but they might be the wrong ones.

It's certainly possible to make it work, but so far I haven't thought about what the minimal/best change to the architecture would look like to achieve that.

[cartercanedy]

I wrapped all of the client network IO in a mutexed block. I can see this getting more efficient if you wanted to adopt a coroutine-oriented api with primitives from async_lock

[cartercanedy]

@birkenfeld I ended up refactoring the background worker logic a bit so that the client can dispatch requests concurrently and receive their responses asynchronously

[cartercanedy]

Hey @birkenfeld just a friendly ping :)

[birkenfeld]

Thanks! This isn't forgotten, I just need a quiet time for review... Fortunately some leave is coming up during which I might have time for that.

[cartercanedy]

No problem, like I said before, feel free to suggest/make changes!

[cartercanedy]

I also had a productive conversation with bhf support about routing in general, and now I'm fairly certain that the Source::Auto variant is pretty much never valid unless you manually configure the routes on the remote (which contradicts the semantics of "auto" routing, imo). I'll create another issue with better details

[cartercanedy]

Hey @birkenfeld, any updates?

[birkenfeld]

Thanks for the ping, should be able to review now!

[cartercanedy]

Let me know what you want to do about the dropped test and inconsistent packet handling. I think I managed to take care of everything else, but please correct me if I'm wrong

[birkenfeld]

Thanks for the changes, looks very nice, just a few points to finish up!

[cartercanedy]

That should be everything, you're good to rerun CI

[birkenfeld]

Great, thanks!

For interest, did you run some benchmarks to see if the changes make a noticeable difference in performance in a single-threaded use case?

[cartercanedy]

I just finished up a test project, I can confirm that the current changes actually increases performance. I'm seeing a 10-15% speedup with the current changes.

Version	Platform	10K Reads	10K Writes
v0.4.4 (crates.io)	Linux	~3000 ms	~3100 ms
cartercanedy/master	Linux	~2700 ms	~2700 ms
Difference		11 %	14 %
v0.4.4 (crates.io)	Windows	~ 1400 1600 ms	~ 1400 1600 ms
cartercanedy/master	Windows	~ 1600 1400 ms	~ 1600 1400 ms
Difference		14 %	14 %

These numbers stayed pretty stable across multiple runs. The linux results were collected in WSL, so I'm pretty sure that virtio overhead is to blame for the unexpected slowdown.

[birkenfeld]

Thank you! So correct me if I'm wrong, the more reliable benchmark (outside WSL) is not showing a speedup? (Although 14% doesn't worry me too much)

Do you have the benchmarking code ready? I'd like to run them on my Linux setup too.

Since we're doing a lot of locking, it might be worth looking at parking_lot mutexes too.

[cartercanedy]

I made the benchmarking code a bit more rigorous, and most of the perf differences are negligible. I pushed the benching code to cartercanedy/master/benches and upstream/master/benches in my repo if you want to pull down yourself. You'll need powershell + .net 10 preview sdk installed to run the script in benches/run-bench.ps1.

The server emu code is cobbled together from the bhf ADS .net samples with some minor code changes of my own. Lmk if you have trouble running them

[cartercanedy]

Thank you! So correct me if I'm wrong, the more reliable benchmark (outside WSL) is not showing a speedup? (Although 14% doesn't worry me too much)

Both Windows and Linux showed a perf improvement using the new code, I was just surprised that the Windows tests were running faster than the Linux tests, since that's usually the other way around.

[cartercanedy]

I just benched against a build using parking_lot mutexes, and there wasn't a significant difference. Since we're not getting any additional perf I'd prefer to stick with using the std impl

[birkenfeld]

Both Windows and Linux showed a perf improvement using the new code

So the 1400/1600 numbers are switched in the table?

The server emu code is cobbled together from the bhf ADS .net samples with some minor code changes of my own.

Oh ok, that will be hard for me to run on Linux I think. But in any case, I think this is good to go, I can try to do some benching/optimizing later if I get the fancies :D

I just benched against a build using parking_lot mutexes, and there wasn't a significant difference.

Thanks for verifying this!

[birkenfeld]

Regarding the merge, are you fine with squashing?

[cartercanedy]

So the 1400/1600 numbers are switched in the table?

Yes, you'll have to forgive my intermittent blindness 🥲

Regarding the merge, are you fine with squashing?

Squashing is fine with me!

[cartercanedy]

I just pushed linux-specific bug fixes to the benching branches. You'll be able to use .net 9, as well, so no worries with trying to figure out how to get the preview sdk set up and working.

[birkenfeld]

Thanks for the patience and getting this over the finish line! :D