-
Notifications
You must be signed in to change notification settings - Fork 46
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
Performance Improvements (and refactoring) #48
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone through the PR and just wrote down the reasons why I did somethings a certain way (specifically the stuff that may not be quite clear). You may want to view these with the diff view, that should make some of them more clear.
@@ -74,7 +75,6 @@ type Server struct { | |||
type Timetag struct { | |||
timeTag uint64 // The acutal time tag | |||
time time.Time | |||
MinValue uint64 // Minimum value of an OSC Time Tag. Is always 1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This results in an unnecessarily larger struct, and is admittedly rather useless, considering it's intended purpose and there's no way to make this value read-only, which means there's no guarantee that the value will remain 1
(which you get with a const
).
strBuf = strBuf[:0] | ||
strBuf = append(strBuf, msg.Address...) | ||
if len(tags) == 0 { | ||
return string(strBuf) | ||
} | ||
|
||
formatString := "%s %s" | ||
var args []interface{} | ||
args = append(args, msg.Address) | ||
args = append(args, tags) | ||
strBuf = append(strBuf, ' ') | ||
strBuf = append(strBuf, tags...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reusing a byte array is far more efficient than appending to a string, as strings are immutable. While reusing a slice can use the same underlying array as far as it goes, only expanding it when necessary.
|
||
// LightMarshalBinary allows you to provide a pre-created `*bytes.Buffer` for MarshalBinary | ||
func (msg *Message) LightMarshalBinary(data *bytes.Buffer) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this function for areas where speed and efficiency is very important (I'm going to assume that's most OSC applications). This allows you to provide a pre-created bytes.Buffer
saving a rather large allocation.
b := initBuf[:data.Len()] | ||
data.Read(b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-using the same buffer allows us to avoid allocating a second one, while not actually using any extra room (thanks to initBuf
, bit of a hack.). However this does have the downside of being a race-condition, so methods to deal with this need to be explored.
if _, err = data.Write(bd); err != nil { | ||
return nil, err | ||
if err := b.Timetag.LightMarshalBinary(data); err != nil { | ||
return err | ||
} | ||
|
||
// Process all OSC Messages | ||
for _, m := range b.Messages { | ||
buf, err := m.MarshalBinary() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanna figure out how to use m.LightMarshalBinary
for this.
if _, err := buf.Write(data); err != nil { | ||
return 0, nil | ||
} | ||
n, _ := buf.Write(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bytes.Buffer.Write()
never returns an error, it only panics.
numPadBytes = n | ||
} | ||
numPadBytes := padBytesNeeded(n) | ||
buf.Write(padBytes[:numPadBytes]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured that instead of creating a new slice with the right length or using a loop and WriteByte
it just made more sense to pre-allocate a []byte
with 4 0
's and only write the amount required..
// bytes are removed from the buffer. | ||
func readPaddedString(buffer *bytes.Buffer) (string, int, error) { | ||
//Read the string from the buffer | ||
str, err := buffer.ReadString(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still haven't found a good substitute for this (and it's horribly expensive).
|
||
return n + numPadBytes, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't need to return an error if we'll never get one, right?
// getTypeTag returns the OSC type tag for the given argument. | ||
func getTypeTag(arg interface{}) (string, error) { | ||
// GetTypeTag returns the OSC type tag for the given argument. | ||
func GetTypeTag(arg interface{}) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figured this would be better public.
@chabad360 are you still planning to complete this PR and merge the improvements? |
I would love to, but I would kinda prefer to split |
@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. |
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. |
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. |
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.
Just to give an idea of the performance improvement [1]:
Server.ReceivePacket()
ParsePacket()
Message.String()
Message.MarshalBinary()
[1]: https://replit.com/@chabad360/go-osc-benchmark