[IMPROVED] Route sid parse performance #478

miraclesu · 2017-04-27T10:32:35Z

Link to issue
Documentation added (if applicable)
Tests added
Branch rebased on top of current master (git pull --rebase origin master)
Changes squashed to a single commit (described here)
Build is green in Travis CI
You have certified that the contribution is your original work and that you license the work to the project under the MIT license

Changes proposed in this pull request:

Base on func (s *Server) routeSidQueueSubscriber(rsid []byte) (*subscription, bool) rsid comes from func routeSid(sub *subscription) string, I think we can replace regexp.FindSubmatch with a simple function.

The Benchmark result:

go test -bench=. -benchmem
BenchmarkRegexp-4   	 1000000	      1124 ns/op	     144 B/op	       2 allocs/op
BenchmarkParse-4    	50000000	        26.2 ns/op	       0 B/op	       0 allocs/op

The Benchmark code:

const (
	RSID  = "RSID"
	QRSID = "QRSID"

	QRSID_LEN = len(QRSID)

	RSID_CID_INDEX   = 1
	RSID_SID_INDEX   = 2
	EXPECTED_MATCHES = 3
)

var qrsidRe = regexp.MustCompile(`QRSID:(\d+):([^\s]+)`)
var _rsid = []byte("QRSID:1:hello")

func BenchmarkRegexp(b *testing.B) {
	for i := 0; i < b.N; i++ {
		if !bytes.HasPrefix(_rsid, []byte(QRSID)) {
			continue
		}
		matches := qrsidRe.FindSubmatch(_rsid)
		if matches == nil || len(matches) != EXPECTED_MATCHES {
			continue
		}
		_, _ = matches[RSID_CID_INDEX], matches[RSID_SID_INDEX]
	}
}

func BenchmarkParse(b *testing.B) {
	for i := 0; i < b.N; i++ {
		cid, sid, ok := parseRouteSid(_rsid)
		if !ok {
			continue
		}

		_, _ = cid, sid
	}
}

func parseRouteSid(rsid []byte) (uint64, []byte, bool) {
	if !bytes.HasPrefix(rsid, []byte(QRSID)) {
		return 0, nil, false
	}

	// We don't care what's char of rsid[QRSID_LEN+1], it should be ':'
	for i, count := QRSID_LEN+1, len(rsid); i < count; i++ {
		switch rsid[i] {
		case ':':
			return uint64(parseInt64(rsid[QRSID_LEN+1 : i])), rsid[i+1:], true
		}
	}
	return 0, nil, true
}

// Ascii numbers 0-9
const (
	asciiZero = 48
	asciiNine = 57
)

// parseInt64 expects decimal positive numbers. We
// return -1 to signal error
func parseInt64(d []byte) (n int64) {
	if len(d) == 0 {
		return -1
	}
	for _, dec := range d {
		if dec < asciiZero || dec > asciiNine {
			return -1
		}
		n = n*10 + (int64(dec) - asciiZero)
	}
	return n
}

/cc @nats-io/core

kozlovic

Thanks for the contribution! If removing regex, I would completely do without the RSID_CID_INDEX and the like and simply have your parse function returns []byte, []byte, bool.
If not, at least maybe make QRSID+":" a constant.

kozlovic · 2017-04-27T15:08:24Z

server/route.go

@@ -491,6 +484,24 @@ func routeSid(sub *subscription) string {
 	return fmt.Sprintf("%s%s:%d:%s", qi, RSID, sub.client.cid, sub.sid)
 }

+func parseRouteSid(rsid []byte) (matches [EXPECTED_MATCHES][]byte, ok bool) {


Since you get rid of regex, why don't return []byte, []byte, bool instead? Why carry the array of matches that is specific to regex?

kozlovic · 2017-04-27T15:09:10Z

server/route.go

@@ -491,6 +484,24 @@ func routeSid(sub *subscription) string {
 	return fmt.Sprintf("%s%s:%d:%s", qi, RSID, sub.client.cid, sub.sid)
 }

+func parseRouteSid(rsid []byte) (matches [EXPECTED_MATCHES][]byte, ok bool) {
+	if !bytes.HasPrefix(rsid, []byte(QRSID+":")) {


Lets make QRSID+":" a constant then, for example QRSID_PREFIX

kozlovic · 2017-04-27T15:10:05Z

server/route.go

+	for i := start; i < len(rsid); i++ {
+		switch rsid[i] {
+		case ':':
+			matches[RSID_CID_INDEX], matches[RSID_SID_INDEX] = rsid[start:i], rsid[i+1:]


If you change to return individual []byte, I would then do return rsid[start:i], rsid[i+1:], true.

kozlovic · 2017-04-27T15:10:19Z

server/route.go

+		}
+	}
+
+	return


return nil, nil, false

kozlovic · 2017-04-27T15:11:47Z

server/route.go

+		return
+	}
+
+	start := len(QRSID + ":")


Even this offset could be made a constant.

coveralls · 2017-04-28T02:47:26Z

Changes Unknown when pulling 3077dab on miraclesu:improve_route_sid_parse into ** on nats-io:master**.

miraclesu · 2017-04-28T03:06:51Z

@kozlovic Thanks for your response! How about the new codes?

I can't repeat the test fail on go1.6.4 on my local environment.

miraclesu · 2017-04-28T03:10:01Z

server/route.go

@@ -491,6 +481,20 @@ func routeSid(sub *subscription) string {
 	return fmt.Sprintf("%s%s:%d:%s", qi, RSID, sub.client.cid, sub.sid)
 }

+func parseRouteSid(rsid []byte) (cid uint64, sid []byte, ok bool) {


I think it's more clearly with use named function parameters :)

I guess it is a matter of taste ;-) I know that @derekcollison (the author of the project) does not like it. I have to say that I myself do not like the return without the values since I always have to do a double take and think of what (and what value) is actually been returned.
The plus that I see of naming the returned parameters are when documenting the function, or if you want to use a defer to act upon the returned value.

kozlovic · 2017-04-28T16:28:17Z

server/route.go

+	for i := QRSID_PREFIX_LEN; i < len(rsid); i++ {
+		switch rsid[i] {
+		case ':':
+			return uint64(parseInt64(rsid[QRSID_PREFIX_LEN:i])), rsid[i+1:], true


If the server would receive a corrupted message, like QRSID:1:, the server would crash. You need to check that there is actually something past the : character. I believe that the previous use of the regex would have returned false and so server would not crash.

Let's add a test for this as well.

I don't understand why the server would crash when receive a corrupted message like QRSID:1:, rsid[i+1:] is reslicing and never out bound of the array.
I had updated the function and added some tests for invalid queue sid, the expects as same with use the regex.

I apologize, you are right, since i will always be <len(rsid), reslicing at i+1 would simply return an empty slice in that case.

Although your code is now equivalent in behavior with the regex, I now realize that we may have a problem and need to discuss this with @derekcollison. It looks to me that as long as we have a QRSID prefix, we should always return ok, no matter if we are able to decode cid and/or sid.

My reasoning is that if we return !ok when for instance getting QRSID:1 2\r\nok\r\n like in one of your test, the server ends up sending to a regular subscriber. I am not sure this is the original intent. As you can see in here, it looks like if the message was for a queue subscription, the server should not attempt to deliver to other subs, even if no actual queue subscriber is found (line 1124).

Let me check with Derek and once we agree we will get back to you (if parseRouteSid should return ok as long as the prefix is found).

Great! Usually the server wouldn't send invalid queue MSG，but everything is possible on Internet :)

I will take a look sometime today.

Yes that is correct, we should always return true if we think it marks a queue subscription via QRSID prefix.

And by prefix, you mean the presence of QRSID, not necessarily QRSID:?

kozlovic

Need to fix the check for array boundaries.

coveralls · 2017-05-06T04:40:02Z

Coverage increased (+0.02%) to 90.884% when pulling f520651 on miraclesu:improve_route_sid_parse into 11d3eed on nats-io:master.

kozlovic

Based on latest conversation, parseRouteSid should return true as long as sid starts with QRSID, which means that messages will not be delivered to regular subscribers.

kozlovic · 2017-05-08T23:07:44Z

server/route.go

-	RSID_CID_INDEX   = 1
-	RSID_SID_INDEX   = 2
-	EXPECTED_MATCHES = 3
+	QRSID_PREFIX     = QRSID + ":"


Based on latest discussion, let's get rid of QRSID_PREFIX and if you want, you could keep QRSID_LEN = len(QRSID).

kozlovic · 2017-05-08T23:07:59Z

server/route.go

+		return 0, nil, false
+	}
+
+	for i := QRSID_PREFIX_LEN; i < len(rsid); i++ {


You could add back:

start := QRSID_LEN+1 for i:=start; i<len(rsid); i++ { ...

kozlovic · 2017-05-08T23:09:15Z

server/route.go

+	for i := QRSID_PREFIX_LEN; i < len(rsid); i++ {
+		switch rsid[i] {
+		case ':':
+			return uint64(parseInt64(rsid[QRSID_PREFIX_LEN:i])), rsid[i+1:], len(rsid[i+1:]) > 0


change back to rsid[start:i] and change last returned value to true

kozlovic · 2017-05-08T23:09:24Z

server/route.go

+			return uint64(parseInt64(rsid[QRSID_PREFIX_LEN:i])), rsid[i+1:], len(rsid[i+1:]) > 0
+		}
+	}
+	return 0, nil, false


Replace with true

kozlovic · 2017-05-08T23:10:10Z

test/routes_test.go

@@ -390,17 +390,26 @@ func TestRouteQueueSemantics(t *testing.T) {
 	routeSend("MSG foo RSID:1:1 2\r\nok\r\n")
 	// Queue group one.
 	routeSend("MSG foo QRSID:1:2 2\r\nok\r\n")
+	// Invlaid queue sid.
+	routeSend("MSG foo QRSID 2\r\nok\r\n")


With the changes, those invalid SID, since they all start with QRSID, should not translate into a message to regular subscribers.

kozlovic · 2017-05-08T23:10:43Z

test/routes_test.go


 	// Use ping roundtrip to make sure its processed.
 	routeSend("PING\r\n")
 	routeExpect(pongRe)

-	// Should be 2 now, 1 for all normal, and one for specific queue subscriber.
-	matches = clientExpectMsgs(2)
+	// Should be 3 now, 1 for all normal, and one for specific queue subscriber,


As explained above, that should not change the number of messages delivered to regular subscribers, so you would need to revert those changes.

We think it marks a queue subscription via QRSID prefix.

miraclesu · 2017-05-09T08:26:32Z

I had updated the codes.

kozlovic · 2017-05-09T15:20:41Z

LGTM

coveralls · 2017-05-10T06:11:02Z

Coverage decreased (-0.02%) to 90.996% when pulling 22e05cc on miraclesu:improve_route_sid_parse into 6a12c21 on nats-io:master.

kozlovic reviewed Apr 27, 2017

View reviewed changes

miraclesu commented Apr 28, 2017

View reviewed changes

kozlovic reviewed Apr 28, 2017

View reviewed changes

kozlovic requested changes Apr 28, 2017

View reviewed changes

miraclesu force-pushed the improve_route_sid_parse branch 2 times, most recently from 8ae8466 to f520651 Compare May 6, 2017 04:35

kozlovic requested changes May 8, 2017

View reviewed changes

miraclesu force-pushed the improve_route_sid_parse branch from f520651 to 22e05cc Compare May 9, 2017 08:06

miraclesu added 3 commits May 9, 2017 16:16

[IMPROVED] Route sid parse performance

29d1573

Add test for invalid queue sid

b570f8d

Revise queue msg action

26ef3f8

We think it marks a queue subscription via QRSID prefix.

miraclesu force-pushed the improve_route_sid_parse branch from 22e05cc to 26ef3f8 Compare May 9, 2017 08:17

kozlovic approved these changes May 9, 2017

View reviewed changes

derekcollison merged commit 01d2f1d into nats-io:master May 9, 2017

miraclesu deleted the improve_route_sid_parse branch May 10, 2017 01:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IMPROVED] Route sid parse performance #478

[IMPROVED] Route sid parse performance #478

miraclesu commented Apr 27, 2017 •

edited

Loading

kozlovic left a comment

kozlovic Apr 27, 2017

kozlovic Apr 27, 2017

kozlovic Apr 27, 2017

kozlovic Apr 27, 2017

kozlovic Apr 27, 2017

coveralls commented Apr 28, 2017

miraclesu commented Apr 28, 2017

miraclesu Apr 28, 2017

kozlovic Apr 28, 2017

kozlovic Apr 28, 2017 •

edited

Loading

derekcollison Apr 30, 2017

miraclesu May 6, 2017 •

edited

Loading

kozlovic May 7, 2017

miraclesu May 8, 2017

derekcollison May 8, 2017

derekcollison May 8, 2017

kozlovic May 8, 2017

derekcollison May 8, 2017

kozlovic left a comment

coveralls commented May 6, 2017

kozlovic left a comment

kozlovic May 8, 2017

kozlovic May 8, 2017

kozlovic May 8, 2017

kozlovic May 8, 2017

kozlovic May 8, 2017

kozlovic May 8, 2017

miraclesu commented May 9, 2017

kozlovic commented May 9, 2017

coveralls commented May 10, 2017

+              		}
+              	}
+              	return

[IMPROVED] Route sid parse performance #478

[IMPROVED] Route sid parse performance #478

Conversation

miraclesu commented Apr 27, 2017 • edited Loading

Changes proposed in this pull request:

kozlovic left a 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

coveralls commented Apr 28, 2017

miraclesu commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miraclesu May 6, 2017 • 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

kozlovic left a comment

Choose a reason for hiding this comment

coveralls commented May 6, 2017

kozlovic left a 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

miraclesu commented May 9, 2017

kozlovic commented May 9, 2017

coveralls commented May 10, 2017

miraclesu commented Apr 27, 2017 •

edited

Loading

kozlovic Apr 28, 2017 •

edited

Loading

miraclesu May 6, 2017 •

edited

Loading