Call custom hosts in parallel

rsalesc commented 3 months ago

I noticed Competitive Companion is getting slower with time, and it's mainly due to the fact that:

The number of custom hosts (ports to send the parsed problems to) has increased significantly;
We issue requests to those hosts serially (N requests, but usually only 1 really matters), regardless of whether they were good or not.

By making these requests in parallel, we can get a ~8x speed-up in contest parsing.

Serial calls:

slow_cc

Parallel calls:

fast_cc

Will be happy to do any other changes deemed necessary to get this through, the speed up was really helpful for me.

EDIT:

It's worth mentioning that I'm using WSL, and I'm not sure the extension is particularly slow because of that. Since it seems the code is like this for a long time, maybe the impact isn't noticeable in most of the cases (well, I expected a few localhost calls to be super fast, even if serialized).

But anyways, I'm getting super slow parses in WSL and this solves that. Maybe other users will benefit from this as well.

touhidurrr commented 3 months ago

so just sending requests asynchronously fixes this i guess?

Edit: Oh, you already fixed it? I thought it was an issue. Good find!

touhidurrr commented 3 months ago

Also, given that we are dealing with localhosts, the request timeout should be something like 500ms or 1s.

jmerle commented 3 months ago

Thanks for the PR, even though I've never had a problem with the serial calls on Ubuntu it's been a while since I've tried it on Windows (and never on WSL). I've made some changes to your branch:

Changed the error handling to be the same as the old sendTask() implementation. If await getHosts() throws, the error should be catched in the same place it used to be catched (i.e. it should lead to MessageAction.SendTaskFailed being sent to the content script). If a host.send() call throws, the error should be ignored.
Ensured the implementation maintains a limit of 6 open connections at any given time, to prevent #38 from becoming an issue again.

I've verified that the sending still works on my Ubuntu system, can you please pull in my changes and check whether you still see similar performance gains on Windows?

The changes I made cover some of the comments made by @touhidurrr, the rest can be ignored:

We are not expecting at least one request to succeed.
A "No clients found" pop-up is out-of-scope for this PR and probably impossible given the fact that there's never been a requirement for clients to send a response after reading the data we send them. If a request times out that doesn't guarantee no client has used the data we sent in the request.
There is already a request timeout of 500ms on these requests (implemented in https://github.com/jmerle/competitive-companion/blob/master/src/hosts/Host.ts), which is configurable in the extension's options. There shouldn't be a need to make changes related to request timeouts in this PR.

rsalesc commented 3 months ago

Cool, thanks, the code looks super clean now, and yeah, I messed up the existing error handling.

Just tested on my side, and I can still se a significant speed-up (just tested with CF 945 and went from ~30s to 7s, which is super reasonable).

EDIT: adding more context, I figured there are a few threads complaining about network speed in WSL2, so that might be coming from it (host communicating with WSL2). Hope that gets fixed at some point on their side.

jmerle commented 2 months ago

Sorry for the delay, I'll publish these changes in a new release later this weekend.

jmerle / competitive-companion

Call custom hosts in parallel #467