perf: cut per-request overhead from eight allocations to one

bench_test.go

@@ -29,6 +29,14 @@ func (writer *discardWriter) Write(data []byte) (int, error) {
 	return len(data), nil
 }
 
+// WriteString is what *http.response offers, so the double offers it too:
+// without it the benchmark measures a string copy no real server makes.
+func (writer *discardWriter) WriteString(data string) (int, error) {
+	writer.written += len(data)
+
+	return len(data), nil
+}
+
 func (writer *discardWriter) WriteHeader(status int) {
 	writer.status = status
 }

call.go

@@ -22,6 +22,15 @@ import (
 
 const sessionCookieName = "govalin-session"
 
+// The response Content-Type is one of a fixed set and "Content-Type" is already
+// the canonical form net/http would derive, so the header is assigned rather
+// than built per response.
+var (
+	contentTypeTextPlain = []string{contenttypes.TextPlain + "; charset=" + charsets.UTF8}
+	contentTypeTextHTML  = []string{contenttypes.TextHTML + "; charset=" + charsets.UTF8}
+	contentTypeJSON      = []string{contenttypes.ApplicationJSON + "; charset=" + charsets.UTF8}
+)
+
 type raw struct {
 	W   *http.ResponseWriter
 	Req *http.Request
@@ -38,49 +47,41 @@ type Call struct {
 	config          *Config
 	status          int
 	bypassLifecycle bool
-	w               *responseWriter
+	w               responseWriter
+	rawWriter       http.ResponseWriter
 	req             *http.Request
 	pathParams      map[string]string
 	bodyBytes       []byte
 	bodyErr         error
 	formParsed      bool
 	formErr         error
-	charset         string
 	session         session.Session
 	Raw             raw // Raw contains the raw request and response
 }
 
-func newCallFromRequest(w http.ResponseWriter, req *http.Request, config *Config, pathParams map[string]string) Call {
-	govalinIDHeader := req.Header[headers.XGovalinID]
-
+func newCallFromRequest(w http.ResponseWriter, req *http.Request, config *Config, pathParams map[string]string) *Call {
 	var uniqueID string
-	if govalinIDHeader == nil {
-		uniqueID = newCallID()
-	} else {
+	if govalinIDHeader := req.Header[headers.XGovalinID]; govalinIDHeader != nil {
 		uniqueID = govalinIDHeader[0]
 	}
 
-	// Even Raw.W goes through the recording writer, so govalin sees a commit whoever made it.
-	recordingWriter := newResponseWriter(w)
-	var rawWriter http.ResponseWriter = recordingWriter
-
-	call := Call{
+	call := &Call{
 		id:              uniqueID,
 		config:          config,
-		w:               recordingWriter,
+		w:               responseWriter{ResponseWriter: w},
 		req:             req,
 		status:          0,
 		bypassLifecycle: false,
 		pathParams:      pathParams,
-		charset:         charsets.UTF8,
-		Raw: raw{
-			W:   &rawWriter,
-			Req: req,
-		},
 	}
 
+	// Even Raw.W goes through the recording writer, so govalin sees a commit whoever
+	// made it. Both point into the call itself, so a request is one allocation.
+	call.rawWriter = &call.w
+	call.Raw = raw{W: &call.rawWriter, Req: req}
+
 	if config.server.sessionsEnabled {
-		initiateSessionFromCall(&call)
+		initiateSessionFromCall(call)
 	}
 
 	return call
@@ -385,8 +386,18 @@ func (call *Call) writtenStatus() int {
 	return call.status
 }
 
-// ID gives an UUIDv4 string that's unique to the call.
+// ID gives an UUIDv4 string that's unique to the call, or the caller's own ID
+// when the request carried an X-Govalin-Id header.
+//
+// The ID is minted on first use, so a request nobody asks the ID of never pays
+// for one; every later call on the same request gets the same string. Like the
+// rest of Call it belongs to the goroutine serving the request, so a handler
+// that fans out should read the ID before it does.
 func (call *Call) ID() string {
+	if call.id == "" {
+		call.id = newCallID()
+	}
+
 	return call.id
 }
 
@@ -442,7 +453,7 @@ func (call *Call) isSecure() bool {
 func (call *Call) Cookie(name string, cookies ...*http.Cookie) (*http.Cookie, error) {
 	if len(cookies) > 0 {
 		cookies[0].Name = name
-		http.SetCookie(call.w, cookies[0])
+		http.SetCookie(&call.w, cookies[0])
 		call.cachePrivate()
 
 		return cookies[0], nil
@@ -642,7 +653,19 @@ func (call *Call) Status(statusCode ...int) int {
 // forbids one is not worth reporting: a handler that produced a body after
 // NotModified matched wasted its own work, and the response is still correct.
 func (call *Call) writeBody(data []byte) {
-	if _, err := call.w.Write(data); err != nil && !errors.Is(err, http.ErrBodyNotAllowed) {
+	_, err := call.w.Write(data)
+	call.reportBodyError(err)
+}
+
+// writeBodyString writes a response body that is already a string, without the
+// copy a []byte conversion would make of it.
+func (call *Call) writeBodyString(text string) {
+	_, err := call.w.WriteString(text)
+	call.reportBodyError(err)
+}
+
+func (call *Call) reportBodyError(err error) {
+	if err != nil && !errors.Is(err, http.ErrBodyNotAllowed) {
 		slog.Error(fmt.Sprintf("Error when trying write to response, %v", err))
 	}
 }
@@ -652,19 +675,19 @@ func (call *Call) writeBody(data []byte) {
 // Text will set the content-type of the response as text/plain and write it to the response.
 // If no other status has been given the response, it will write a 200 OK to the response.
 func (call *Call) Text(text string) {
-	call.w.Header().Add(headers.ContentType, headers.ContentTypeHeader(contenttypes.TextPlain, call.charset))
+	call.w.Header()[headers.ContentType] = contentTypeTextPlain
 	call.sendStatusOrDefault()
-	call.writeBody([]byte(text))
+	call.writeBodyString(text)
 }
 
 // Send text as HTML to response
 //
 // HTML will set the content-type of the response as text/html and write it to the response.
 // If no other status has been given the response, it will write a 200 OK to the response.
 func (call *Call) HTML(text string) {
-	call.w.Header().Add(headers.ContentType, headers.ContentTypeHeader(contenttypes.TextHTML, call.charset))
+	call.w.Header()[headers.ContentType] = contentTypeTextHTML
 	call.sendStatusOrDefault()
-	call.writeBody([]byte(text))
+	call.writeBodyString(text)
 }
 
 // Send obj as JSON to response
@@ -673,7 +696,7 @@ func (call *Call) HTML(text string) {
 // object as JSON, and writes it to the response. If no other status has been given the response,
 // it will write a 200 OK to the response.
 func (call *Call) JSON(obj interface{}) {
-	call.w.Header().Add(headers.ContentType, headers.ContentTypeHeader(contenttypes.ApplicationJSON, charsets.UTF8))
+	call.w.Header()[headers.ContentType] = contentTypeJSON
 	jsonBytes, err := json.Marshal(obj)
 	if err != nil {
 		slog.Error(fmt.Sprintf("error when trying to JSON marshall object, %v", err))

call_test.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/pkkummermo/govalin"
 	"github.com/pkkummermo/govalin/govalintest"
+	"github.com/pkkummermo/govalin/internal/http/contenttypes"
 	"github.com/pkkummermo/govalin/internal/http/headers"
 	"github.com/stretchr/testify/assert"
 )
@@ -403,6 +404,22 @@ func TestRequestID(t *testing.T) {
 	})
 }
 
+// TestRequestIDIsStableAcrossReads covers the ID being minted on first use: a
+// handler that reads it more than once gets one ID, not one per read.
+func TestRequestIDIsStableAcrossReads(t *testing.T) {
+	app := newTestApp()
+	app.Get("/govalin", func(call *govalin.Call) {
+		call.Text(call.ID() + " " + call.ID())
+	})
+
+	govalintest.Test(t, app, func(client *govalintest.Client) {
+		reads := strings.Fields(client.Get("/govalin"))
+
+		assert.Len(t, reads, 2, "Should have read the ID twice")
+		assert.Equal(t, reads[0], reads[1], "Should give the same ID on every read of a call")
+	})
+}
+
 func TestRedirect(t *testing.T) {
 	app := newTestApp()
 	app.Get("/govalin", func(call *govalin.Call) {
@@ -653,3 +670,25 @@ func TestUninferableBodyTargetDoesNotCompile(t *testing.T) {
 	assert.Contains(t, string(output), "type user of u does not match *T")
 	assert.Contains(t, string(output), "in call to call.BodyAs, cannot infer T")
 }
+
+// TestBodyWriterReplacesAnExistingContentType covers a handler that set the
+// content type itself before writing: the response carries the one the body
+// writer chose, not both.
+func TestBodyWriterReplacesAnExistingContentType(t *testing.T) {
+	app := newTestApp()
+	app.Get("/greeting", func(call *govalin.Call) {
+		call.Header(headers.ContentType, contenttypes.ApplicationJSON)
+		call.Text("hei")
+	})
+
+	govalintest.Test(t, app, func(client *govalintest.Client) {
+		response := client.GetResponse("/greeting")
+
+		assert.Equal(
+			t,
+			[]string{contenttypes.TextPlain + "; charset=utf-8"},
+			response.Header.Values(headers.ContentType),
+			"Should send the content type of the body it actually wrote, and only that one",
+		)
+	})
+}

internal/http/headers/headers.go

@@ -77,8 +77,3 @@ const (
 	XHttpMethodOverride           = "X-HTTP-Method-Override"
 	XPermittedCrossDomainPolicies = "X-Permitted-Cross-Domain-Policies"
 )
-
-// ContentTypeHeader generates a Content-Type header value based on given content type and charset.
-func ContentTypeHeader(contentType string, charset string) string {
-	return contentType + "; charset=" + charset
-}

perf_test.go

@@ -31,13 +31,13 @@ var allocationBudgets = []allocationBudget{
 	{
 		name:    "text",
 		target:  "/text",
-		allowed: 8,
+		allowed: 1,
 		build:   func(app *App) { app.Get("/text", func(call *Call) { call.Text("Hello world") }) },
 	},
 	{
 		name:    "json",
 		target:  "/json",
-		allowed: 9,
+		allowed: 3,
 		build: func(app *App) {
 			type payload struct {
 				Name  string `json:"name"`
@@ -50,13 +50,13 @@ var allocationBudgets = []allocationBudget{
 	{
 		name:    "status only",
 		target:  "/status",
-		allowed: 5,
+		allowed: 1,
 		build:   func(app *App) { app.Get("/status", func(call *Call) { call.Status(http.StatusNoContent) }) },
 	},
 	{
 		name:    "before and after handlers",
 		target:  "/text",
-		allowed: 8,
+		allowed: 1,
 		build: func(app *App) {
 			app.Before("/*", func(_ *Call) bool { return true })
 			app.Get("/text", func(call *Call) { call.Text("Hello world") })
@@ -66,13 +66,13 @@ var allocationBudgets = []allocationBudget{
 	{
 		name:    "not found",
 		target:  "/missing",
-		allowed: 33,
+		allowed: 27,
 		build:   func(app *App) { app.Get("/text", func(call *Call) { call.Text("Hello world") }) },
 	},
 	{
 		name:    "raw handler",
 		target:  "/raw",
-		allowed: 6,
+		allowed: 2,
 		build: func(app *App) {
 			app.HTTPServe("/raw", func(w http.ResponseWriter, _ *http.Request) {
 				_, _ = w.Write([]byte("Hello world"))
@@ -82,7 +82,7 @@ var allocationBudgets = []allocationBudget{
 	{
 		name:    "static file",
 		target:  "/static/sub/test.html",
-		allowed: 39,
+		allowed: 35,
 		build: func(app *App) {
 			app.Static("/static", func(_ *Call, staticConfig *StaticConfig) {
 				staticConfig.WithStaticPath("internal/testdata/static")

responsewriter.go

@@ -20,10 +20,6 @@ type responseWriter struct {
 	committed bool
 }
 
-func newResponseWriter(writer http.ResponseWriter) *responseWriter {
-	return &responseWriter{ResponseWriter: writer}
-}
-
 // WriteHeader records the status and passes it on unchanged. A repeated call is
 // still forwarded, so net/http keeps reporting a genuine double write; the
 // framework's own status flush is guarded by the committed flag instead.
@@ -44,6 +40,19 @@ func (writer *responseWriter) Write(data []byte) (int, error) {
 	return writer.ResponseWriter.Write(data)
 }
 
+// WriteString keeps net/http's string path: *http.response implements
+// io.StringWriter, and without this a string body would be copied into a byte
+// slice of its own before every write.
+func (writer *responseWriter) WriteString(data string) (int, error) {
+	writer.markCommitted()
+
+	if stringWriter, ok := writer.ResponseWriter.(io.StringWriter); ok {
+		return stringWriter.WriteString(data)
+	}
+
+	return writer.ResponseWriter.Write([]byte(data))
+}
+
 // ReadFrom keeps net/http's sendfile path: io.Copy prefers an io.ReaderFrom, and
 // without this every streamed body would fall back to a buffered copy.
 func (writer *responseWriter) ReadFrom(reader io.Reader) (int64, error) {

responsewriter_test.go

@@ -58,6 +58,12 @@ func TestResponseWriterTracksCommitment(t *testing.T) {
 			committed: true,
 			status:    http.StatusOK,
 		},
+		{
+			name:      "a WriteString commits an implicit 200",
+			act:       func(writer *responseWriter) { _, _ = writer.WriteString("body") },
+			committed: true,
+			status:    http.StatusOK,
+		},
 		{
 			name:      "a ReadFrom commits an implicit 200",
 			act:       func(writer *responseWriter) { _, _ = writer.ReadFrom(strings.NewReader("body")) },
@@ -86,7 +92,7 @@ func TestResponseWriterTracksCommitment(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			writer := newResponseWriter(httptest.NewRecorder())
+			writer := &responseWriter{ResponseWriter: httptest.NewRecorder()}
 
 			test.act(writer)
 
@@ -99,3 +105,23 @@ func TestResponseWriterTracksCommitment(t *testing.T) {
 		})
 	}
 }
+
+// writeOnly hides the WriteString a recorder happens to have, leaving the plain
+// http.ResponseWriter that third-party middleware may hand the wrapper.
+type writeOnly struct {
+	http.ResponseWriter
+}
+
+// TestResponseWriterWritesStringsWithoutAStringWriter covers the wrapped writer
+// not taking strings: the body still has to reach it.
+func TestResponseWriterWritesStringsWithoutAStringWriter(t *testing.T) {
+	recorder := httptest.NewRecorder()
+	writer := &responseWriter{ResponseWriter: writeOnly{recorder}}
+
+	if _, err := writer.WriteString("body"); err != nil {
+		t.Fatalf("WriteString returned %v", err)
+	}
+	if recorder.Body.String() != "body" {
+		t.Errorf("body is %q, expected %q", recorder.Body.String(), "body")
+	}
+}