Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add go version to increase performance #1

Merged
merged 4 commits into from
Oct 25, 2022

Conversation

Night12138
Copy link
Collaborator

latency jitter 50ms=>20ms

@tamilpp25
Copy link
Member

I got to tell I have no experience in go but this looks really well written compared to the python one, like don't have to relaunch proxy when switching server, and the ping is low too, but I did run into a few issues while running this:

  • First time login takes quite some time and KCP seems to reestablish 2-3 times and sometimes is stuck at the end of the element loading screen
  • KCP doesn't close connection sometimes when quitting game (this never worked on python so I just had it save file on exiting program)
  • Proxy can only save dump on connection close, which doesn't close sometimes as stated above, so possible to make it close on quitting the program?
  • Packets don't seem to get processed while playing the game and client attempts to reconnect after like few seconds of entering the game

@Night12138
Copy link
Collaborator Author

@tamilpp25 I'm grateful that you take time to review it! (maybe just have a try ) I will try to reproduce the problem at night and fix some unreasonable parts (such as strong reliance on handshake packets to close)

@tamilpp25
Copy link
Member

tamilpp25 commented Oct 24, 2022

Issue I saw with packets not processing was, once you enter game it works fine for like 10-20 seconds but after that it won't and ends up trying to reconnect, probably issue with kcp? as this has happened in python version multiple times like the kcp crashes midway

@Night12138
Copy link
Collaborator Author

Night12138 commented Oct 24, 2022

Hi @tamilpp25, glad to sync the progress with you now. With some work I solved a bunch of issues and it's now running on my computer with amazing stability (only 2 ms latency). You can check the change with this commit.
Something missuses in my case:

  1. Didn't set NoDelay for the client, but I have no idea why setting NoDelay on your python version will cause the crash.
  2. Misuse of update() and check(), using the same current time will solve the KCP always try to re-establish the connection.

BTW, now you can dump all packets when quitting server :-D.

image

Have fun!

@tamilpp25
Copy link
Member

tamilpp25 commented Oct 24, 2022

This is really awesome, ping is greatly reduced, and packets seem to be processed right on time but after playing for a while it looks like the kcp crashes and I get this error, this happened twice like after 2-3mins of entering game and playing around

Exception 0xc0000005 0x0 0x12c42abf000 0x7ff934c89749
PC=0x7ff934c89749
signal arrived during external code execution

also this is where the error seem to happen:

created by github.com/MoonlightPS/Iridium-gidra/gover/proxy.(*KCPConn).Start
        D:/Stuff/ps/iridium-gidra/gover/proxy/socket.go:81 +0x97

after which the program exits with exit status 2

@Night12138
Copy link
Collaborator Author

Night12138 commented Oct 24, 2022

Unfortunately, this seems to be a problem with goroutine racing, maybe the KCP object is not concurrency/multi-threading safe, so I may need to lock the KCP object when reading and writing operations, which may cause refactoring... I'll watch at this issue and make some goroutine race tests tomorrow night, thanks for the feedback!

@Night12138
Copy link
Collaborator Author

Okay... KCP is not multi-threading safe, quote for skywind3000/kcp#190 (comment) and skywind3000/kcp#207, It may be the same reason for your python version crash too. I will refactor that tomorrow, now just go to bed :-P

@Night12138
Copy link
Collaborator Author

Night12138 commented Oct 25, 2022

Hi @tamilpp25, I added a mutex to the operation of the KCP object in a simple way, and the game ran stably for 10+ minutes. This doesn't seem to affect performance much so I didn't refactor in a lock-free way. Please check if the issue still exists when you are free. You may be interest to check the change with this commit.

@tamilpp25
Copy link
Member

Tested for almost half an hour, the KCP doesn't crash and the gameplay is smooth 👍🏻
I might as well just replace the python version with this instead of having 2 proxies

@tamilpp25
Copy link
Member

also I'd like you to add a few more parameters while saving the dump file so that it matches the format

[
  {
    "index": int,
    "packetId": int,
    "protoName": string,
    "source": string,
    "time": float,
    "object": protobuf object
  }
]

@Night12138
Copy link
Collaborator Author

I will update the record format tonight, may you do more tests to find more issues I can fix together?

@tamilpp25
Copy link
Member

sure

gover/utils/recorder.go Outdated Show resolved Hide resolved
@Night12138
Copy link
Collaborator Author

Hi @tamilpp25, I have updated the record format in this commit.

@tamilpp25
Copy link
Member

Awesome, looks good to me and nice logger too 👍🏻

Copy link
Member

@tamilpp25 tamilpp25 left a comment

Choose a reason for hiding this comment

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

TODO for me:

  • Use this as main and remove the python version later

@tamilpp25 tamilpp25 merged commit 4176d8a into MoonlightPS:master Oct 25, 2022
@Night12138
Copy link
Collaborator Author

TODO for me:

  • Use this as main and remove the python version later

BTW, I have no experience in reversing the mhy protocol and I may be overwhelmed once mhy has updated their protocol, so maybe you may keep your python version for a while and I can keep working on translating your python code until you have got enough experience in go. :-D

@tamilpp25
Copy link
Member

Alright sure, If you want to contact me message on discord: tamilpp25#9288

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