search

hypebeast / go-osc

Open Sound Control (OSC) library for Golang. Implemented in pure Go.
MIT License
202 stars 46 forks source link

Performance Improvements (and refactoring) #48

Open chabad360 opened 3 years ago

chabad360 commented 3 years ago

Here are a large number of performance improvements. I want to move the discussion over from #47 as it's not possible to comment on actual code in an issue.

This code is not intended to be merged, while it is functional, it is primarily for discussion. I would rather wait until the repo has been restructured into separate files, as that will make it easier to digest the changes.

Note: the current changes in this patch are not the changes that are shown in this benchmark, those are from my master branch (warning, I've modified the api there)

Just to give an idea of the performance improvement [1]:

Function ops ↑ ns/op ↓ B/op ↓ allocs/op ↓
Server.ReceivePacket()
Original 23,005 60,837 70,080 18
New 1,000,000 1,911 208 8
ParsePacket()
Original 121,566 8,235 4,624 18
New 1,000,000 1,303 192 7
Message.String()
Original 804,878 3,510 256 10
New 1,000,000 1560 83 2
Message.MarshalBinary()
Original 1,000,000 1,942 368 10
New 1,000,000 1,017 83 2
New (Light Version) 3,024,367 498 3 1

[1]: https://replit.com/@chabad360/go-osc-benchmark

chabad360 commented 3 years ago

@hypebeast?

giohappy commented 3 years ago

@chabad360 are you still planning to complete this PR and merge the improvements?

chabad360 commented 3 years ago

I would love to, but I would kinda prefer to split osc.go into multiple separate files before finishing this up. It's a lot of changes, so it would be better to push this once the changes are more clear. However, if @hypebeast would prefer to do this first, I'll gladly finish it up.

chabad360 commented 2 years ago

@giohappy and anyone else who's interested in this: I would recommend trying to use my fork, it has some api changes (that I believe are sane and make it more idiomatic), I'm actively maintaining it, so at least I'll be more quick to fix bugs.

depili commented 2 years ago

I have tried this fork, and sadly this isn't functional in a real world environment, as the decoder panics on empty strings. In general issuing fatal errors on decoding of invalid packets on a server is a door for DoS attacks and should be avoided if possible.

The forked repository also doesn't have issue tracker enabled for reporting such issues with it.

chabad360 commented 2 years ago

Oh, yes. I know about that, it bit me in back side pretty hard already. Very poor decision on my part (I don't actually remember why I made it do that). I used a temporary fix in my application, but in a week or two I'm going to be going on a bit of a bug hunt/major rewrite. So it'll be fixed then.

I'll enable issue tracker later today.