search

ThePrimeagen / tyrone-biggums

Clearly a repo about websockets and their comparison...
Other
465 stars 56 forks source link

Incremental refactor of RxJS implementation. #4

Open benlesh opened 2 years ago

benlesh commented 2 years ago

While the overall diff might look like a complete rewrite, I stuck with your approach and simply made incremental changes, commit-by-commit. Overall this is still not how I'd implement a server (if RxJS and Node were requirements for some reason), it's just a refactor of what's here. See commits for details.

High Level:

  1. There were several places where you were creating subscriptions but not unsubscribing them or cleaning them up.
  2. There was a LOT of unnecessary RxJS in this example. As an example, there were a couple of cases where a Subject was being used to create a unicast observable, or to wrap what was already multicast with an observable.
  3. There were other unnecessary abstractions I'd have steered clear of that might also be hurting your callbacks implementation.
  4. The tests never worked for me. Two were broken out of the gate. I deleted the rxjs-related tests that no longer align with the implementation. I didn't add new tests, however. If you want to test those, the function that create the Observables are exported from the modules they live in, and you can test those in isolation provided mocks of things like sockets, etc.
  5. I was unable to run any of your tests. In your videos it seems you're running some Netflix-specfic python script under ~/work when you spin up your docker image.

As a whole, this example would make a great sample to use to showcase incremental refactoring of code that wasn't meeting requirements. In all, it didn't even take that long. Incremental refactor is a skill I can't recommend enough.

There may be some argument that "I didn't do this the RxJS way". But there is no RxJS way. I can't control what weird tutorials or explainers are out in the wild. It's just a set of tools.

All of that said:

  1. IMO, I would never implement a "high speed game server", especially one this simple, in Node. 🤷
  2. Callbacks (aka no/little abstraction) will always beat any additional abstraction. Especially if that abstraction isn't well leveraged.
  3. This is probably still slower than callbacks. I'm sure you'll editorialize that on YouTube or Twitter as "A disaster" or a "toilet fire" (your exact words), but do what you will.

At this point the ball is in your court. I'm attempting to help you with your comparison, good luck with your YouTube channel.

ThePrimeagen commented 2 years ago

Its exceptionally hard to tell what has changed due to indenting changes (4 -> 2).

Which files were most bestest refactored?

benlesh commented 2 years ago

Ah... Sorry, I have things set to auto format and I figured you had a prettier config it was honoring.

This might help:

image

benlesh commented 2 years ago

I've added comments to help call out some of the more significant parts of the refactor.

benlesh commented 2 years ago

Are you able to merge this and test it? I'm curious if this improves the outcome a little. I don't think it will be faster than anything callback based, but I'd like to see the results. I don't have the Netflix-specific script you're running to get these numbers. Is this something that can be open sourced? Or is it proprietary Netflix stuff?

ThePrimeagen commented 2 years ago

It's one hundred percent not proprietary. It's just the docker file. I did run it and it seemed to run in similar speeds. I did not do any deeper dive memory or performance. But eyeball statistics suggests that it's near similar. But eyeball statistics are always misleading.

You have respectfully requested that I leave things alone, so I'm just leaving things alone. I have continued with memory pooling and other memory reducing techniques which seem to show some promise.

I meant to keep this PR up, as I may come back to it and try to pull out the upgrades and validate it in a more rigorous way.

benlesh commented 2 years ago

I see. Can you please explain, in detail, how to run the profiles you're running? I'd like to see the differences myself. If there's something I can learn to improve the library, I will.

ThePrimeagen commented 2 years ago

In the process of writing the test client in rust for more accurate results. Its easy for the clock to drift in JS and to start sending fire commands slower than every 200ms.

Therefore I am rewriting it. Once done, i'll try to remember to update this issue with the proper way to run it. I'll also include the tester as a docker container so its easy to get results out.

ThePrimeagen commented 2 years ago

Lastly, you will also need a linode instance (or some other company). I would suggest a single CPU that way the effects are "more" realistic.