Repository: gorilla/mux
Pull request #732 (2023-08-24 13:03:38)

Optimize/new route regexp allocations
## What type of PR is this? (check all applicable)

- [x] Refactor
- [ ] Feature
- [ ] Bug Fix
- [x] Optimization
- [ ] Documentation Update

## Description

This optimizes NewRouter creation. The change introduces strings.Builder for more efficient strings concatenation, and replaces SplitN with Index, avoiding the allocations for the slice.

## Related Tickets & Documents

| Test Group                  | Condition | Average Time (ns/op) | Average Memory (B/op) | Average Allocations |
|-----------------------------|-----------|----------------------|-----------------------|---------------------|
| **BenchmarkNewRouter**      | Before    | 37041                | 26553                  | 376                 |
|                             | After     | 32046                | 25800                  | 347                 |
| **BenchmarkNewRouterRegexpFunc** | Before | 4713                 | 2385                   | 75                  |
|                             | After     | 2792                 | 1640                   | 46                  |


BenchmarkNewRouterRegexpFunc:

Time Improvement: Significant improvement in execution time from 4,713 ns/op to 2,792 ns/op.
Memory Usage: Memory usage per operation dropped from 2,385 bytes to 1,640 bytes.
Allocations: Allocations per operation decreased substantially from 75 to 46.

## Added/updated tests?

- [x] Yes

## Run verifications and test

- [ ] `make verify` is passing
- [x] `make test` is passing


titpetric: Rebased off #731 
titpetric: @AlexVulaj thanks for #731 merge; rebased this one for review. Benchmark still the same as in description.
titpetric: @coreydaley is there anything i can/should do to get this reviewed?
apoorvajagtap: Hi @titpetric, thanks for working on this! 
Also, thanks for being patient while we were adapting to the changes :). I have looked through & added some inputs, please feel free to share your thoughts.
apoorvajagtap: @titpetric LGTM.
Would you like to squash the commits please? (mainly those that are very related and just have slight differences)
apoorvajagtap: The lint & security scan errors are not related to the current changes, so I'll go ahead with squash & merge. 

Patch:
diff --git a/regexp.go b/regexp.go
index 8a1ed86..e0bcff6 100644
--- a/regexp.go
+++ b/regexp.go
@@ -65,37 +65,50 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro
 	}
 	varsN := make([]string, len(idxs)/2)
 	varsR := make([]*regexp.Regexp, len(idxs)/2)
-	pattern := bytes.NewBufferString("")
+
+	var pattern, reverse strings.Builder
 	pattern.WriteByte('^')
-	reverse := bytes.NewBufferString("")
-	var end int
+
+	var end, colonIdx, groupIdx int
 	var err error
+	var patt, param, name string
 	for i := 0; i < len(idxs); i += 2 {
 		// Set all values we are interested in.
+		groupIdx = i / 2
+
 		raw := tpl[end:idxs[i]]
 		end = idxs[i+1]
-		parts := strings.SplitN(tpl[idxs[i]+1:end-1], ":", 2)
-		name := parts[0]
-		patt := defaultPattern
-		if len(parts) == 2 {
-			patt = parts[1]
+		tag := tpl[idxs[i]:end]
+
+		// trim braces from tag
+		param = tag[1 : len(tag)-1]
+
+		colonIdx = strings.Index(param, ":")
+		if colonIdx == -1 {
+			name = param
+			patt = defaultPattern
+		} else {
+			name = param[0:colonIdx]
+			patt = param[colonIdx+1:]
 		}
+
 		// Name or pattern can't be empty.
 		if name == "" || patt == "" {
-			return nil, fmt.Errorf("mux: missing name or pattern in %q",
-				tpl[idxs[i]:end])
+			return nil, fmt.Errorf("mux: missing name or pattern in %q", tag)
 		}
 		// Build the regexp pattern.
-		fmt.Fprintf(pattern, "%s(?P<%s>%s)", regexp.QuoteMeta(raw), varGroupName(i/2), patt)
+		groupName := varGroupName(groupIdx)
+
+		pattern.WriteString(regexp.QuoteMeta(raw) + "(?P<" + groupName + ">" + patt + ")")
 
 		// Build the reverse template.
-		fmt.Fprintf(reverse, "%s%%s", raw)
+		reverse.WriteString(raw + "%s")
 
 		// Append variable name and compiled pattern.
-		varsN[i/2] = name
-		varsR[i/2], err = RegexpCompileFunc(fmt.Sprintf("^%s$", patt))
+		varsN[groupIdx] = name
+		varsR[groupIdx], err = RegexpCompileFunc("^" + patt + "$")
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("mux: error compiling regex for %q: %w", tag, err)
 		}
 	}
 	// Add the remaining.
@@ -114,18 +127,9 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro
 		pattern.WriteByte('$')
 	}
 
-	var wildcardHostPort bool
-	if typ == regexpTypeHost {
-		if !strings.Contains(pattern.String(), ":") {
-			wildcardHostPort = true
-		}
-	}
-	reverse.WriteString(raw)
-	if endSlash {
-		reverse.WriteByte('/')
-	}
 	// Compile full regexp.
-	reg, errCompile := RegexpCompileFunc(pattern.String())
+	patternStr := pattern.String()
+	reg, errCompile := RegexpCompileFunc(patternStr)
 	if errCompile != nil {
 		return nil, errCompile
 	}
@@ -136,6 +140,17 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro
 			"Only non-capturing groups are accepted: e.g. (?:pattern) instead of (pattern)")
 	}
 
+	var wildcardHostPort bool
+	if typ == regexpTypeHost {
+		if !strings.Contains(patternStr, ":") {
+			wildcardHostPort = true
+		}
+	}
+	reverse.WriteString(raw)
+	if endSlash {
+		reverse.WriteByte('/')
+	}
+
 	// Done!
 	return &routeRegexp{
 		template:         template,


Test patch:
diff --git a/regexp_test.go b/regexp_test.go
index d7518f3..f7fce81 100644
--- a/regexp_test.go
+++ b/regexp_test.go
@@ -4,9 +4,35 @@ import (
 	"net/url"
 	"reflect"
 	"strconv"
+	"strings"
 	"testing"
 )
 
+func Test_newRouteRegexp_Errors(t *testing.T) {
+	tests := []struct {
+		in, out string
+	}{
+		{"/{}", `mux: missing name or pattern in "{}"`},
+		{"/{:.}", `mux: missing name or pattern in "{:.}"`},
+		{"/{a:}", `mux: missing name or pattern in "{a:}"`},
+		{"/{id:abc(}", `mux: error compiling regex for "{id:abc(}":`},
+	}
+
+	for _, tc := range tests {
+		t.Run("Test case for "+tc.in, func(t *testing.T) {
+			_, err := newRouteRegexp(tc.in, 0, routeRegexpOptions{})
+			if err != nil {
+				if strings.HasPrefix(err.Error(), tc.out) {
+					return
+				}
+				t.Errorf("Resulting error does not contain %q as expected, error: %s", tc.out, err)
+			} else {
+				t.Error("Expected error, got nil")
+			}
+		})
+	}
+}
+
 func Test_findFirstQueryKey(t *testing.T) {
 	tests := []string{
 		"a=1&b=2",