Performance Improvements (and refactoring) #48

chabad360 · 2021-03-22T18:45:27Z

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

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.

chabad360 · 2021-03-30T19:44:50Z

osc/osc.go

@@ -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.


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).

chabad360 · 2021-03-30T19:52:43Z

osc/osc.go

+	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...)


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.

chabad360 · 2021-03-30T19:55:23Z

osc/osc.go


+// LightMarshalBinary allows you to provide a pre-created `*bytes.Buffer` for MarshalBinary
+func (msg *Message) LightMarshalBinary(data *bytes.Buffer) error {


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.

chabad360 · 2021-03-30T19:58:54Z

osc/osc.go

+	b := initBuf[:data.Len()]
+	data.Read(b)


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.

chabad360 · 2021-03-30T19:59:39Z

osc/osc.go

-	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()


I wanna figure out how to use m.LightMarshalBinary for this.

chabad360 · 2021-03-30T20:20:23Z

osc/osc.go

-	if _, err := buf.Write(data); err != nil {
-		return 0, nil
-	}
+	n, _ := buf.Write(data)


bytes.Buffer.Write() never returns an error, it only panics.

chabad360 · 2021-03-30T20:23:28Z

osc/osc.go

-		numPadBytes = n
-	}
+	numPadBytes := padBytesNeeded(n)
+	buf.Write(padBytes[:numPadBytes])


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..

chabad360 · 2021-03-30T20:24:07Z

osc/osc.go

+// 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)


Still haven't found a good substitute for this (and it's horribly expensive).

chabad360 · 2021-03-30T20:26:26Z

osc/osc.go


-	return n + numPadBytes, nil


don't need to return an error if we'll never get one, right?

chabad360 · 2021-03-30T20:26:43Z

osc/osc.go

-// 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) {


Figured this would be better public.

chabad360 · 2021-04-20T04:32:52Z

@hypebeast?

giohappy · 2021-10-07T15:06:39Z

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

chabad360 · 2021-10-08T02:56:34Z

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 · 2021-11-17T00:24:08Z

@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 · 2022-01-26T09:07:00Z

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 · 2022-01-26T16:42:32Z

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.

chabad360 added 10 commits March 12, 2021 13:02

significatly reduce allocations

da89c1c

changes to reading the first byte

b6ddc28

merge the changes from master

2a5d6ae

Message.TypeTags(): return if there are no arguments

25cefca

oops

8e48709

fix another oops

aabd942

small things

dd63512

some little things

08b6fea

Update osc.go

1bf016c

more optimizations

1711540

chabad360 mentioned this pull request Mar 22, 2021

Performance could be a lot better #47

Open

chabad360 changed the title ~~Performance Improvements~~ Performance Improvements (and refactoring) Mar 30, 2021

chabad360 added 4 commits March 30, 2021 19:42

refactor

32e7011

small optimization

0f219b5

small thing

4f26d38

n1 is useless

d12342e

chabad360 commented Mar 30, 2021

View reviewed changes

Merge branch 'master' into patch-1

cc33749

hypebeast mentioned this pull request Apr 26, 2022

TCP support (take 2) #42

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance Improvements (and refactoring) #48

Performance Improvements (and refactoring) #48

chabad360 commented Mar 22, 2021 •

edited

Loading

chabad360 left a comment •

edited

Loading

chabad360 Mar 30, 2021 •

edited

Loading

chabad360 Mar 30, 2021

chabad360 Mar 30, 2021

chabad360 Mar 30, 2021

chabad360 Mar 30, 2021

chabad360 Mar 30, 2021

chabad360 Mar 30, 2021

chabad360 Mar 30, 2021

chabad360 Mar 30, 2021

chabad360 Mar 30, 2021

chabad360 commented Apr 20, 2021

giohappy commented Oct 7, 2021

chabad360 commented Oct 8, 2021

chabad360 commented Nov 17, 2021

depili commented Jan 26, 2022

chabad360 commented Jan 26, 2022 •

edited

Loading


		// LightMarshalBinary allows you to provide a pre-created `*bytes.Buffer` for MarshalBinary
		func (msg Message) LightMarshalBinary(data bytes.Buffer) error {

Performance Improvements (and refactoring) #48

Are you sure you want to change the base?

Performance Improvements (and refactoring) #48

Conversation

chabad360 commented Mar 22, 2021 • edited Loading

chabad360 left a comment • edited Loading

Choose a reason for hiding this comment

chabad360 Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chabad360 commented Apr 20, 2021

giohappy commented Oct 7, 2021

chabad360 commented Oct 8, 2021

chabad360 commented Nov 17, 2021

depili commented Jan 26, 2022

chabad360 commented Jan 26, 2022 • edited Loading

chabad360 commented Mar 22, 2021 •

edited

Loading

chabad360 left a comment •

edited

Loading

chabad360 Mar 30, 2021 •

edited

Loading

chabad360 commented Jan 26, 2022 •

edited

Loading