feat: add header.PeekAll #1394

li-jin-gou · 2022-10-12T07:54:41Z

pr for #179

erikdubbelboer · 2022-10-12T16:01:16Z

args.go

@@ -105,7 +105,7 @@ func (a *Args) ParseBytes(b []byte) {

 // String returns string representation of query args.
 func (a *Args) String() string {
-	return string(a.QueryString())
+	return b2s(a.QueryString())


I don't think we should make a backwards incompatible change like this.

erikdubbelboer · 2022-10-12T16:05:28Z

args.go

@@ -260,7 +260,7 @@ func (a *Args) PeekBytes(key []byte) []byte {
 func (a *Args) PeekMulti(key string) [][]byte {
 	var values [][]byte
 	a.VisitAll(func(k, v []byte) {
-		if string(k) == key {
+		if b2s(k) == key {


This is not needed. string(bv) == sv is optimized in the compiler and doesn't allocate: https://go.dev/play/p/WYcbS08dv_a
I haven't tested it but I wouldn't be surprised if it's even faster than with b2s.

erikdubbelboer · 2022-10-12T16:07:20Z

args.go

@@ -343,7 +343,7 @@ func (a *Args) GetUfloatOrZero(key string) float64 {
 // true is returned for "1", "t", "T", "true", "TRUE", "True", "y", "yes", "Y", "YES", "Yes",
 // otherwise false is returned.
 func (a *Args) GetBool(key string) bool {
-	switch string(a.Peek(key)) {
+	switch b2s(a.Peek(key)) {


Same here, this is also already optimized by the compiler not to allocate: https://go.dev/play/p/fkD2_0estoT

erikdubbelboer · 2022-10-12T16:09:04Z

header.go

@@ -1793,6 +1793,69 @@ func (h *RequestHeader) peek(key []byte) []byte {
 	}
 }

+// PeekAll returns header value for the given key.
+func (h *RequestHeader) PeekAll(key string) [][]byte {
+	k := getHeaderKeyBytes(&h.bufKV, key, h.disableNormalizing)


I wonder if we should do the same trick here as with bufKV. Where we put a [][]byte in RequestHeader and reuse that instead of allocating the return value on the heap.

thanks,You're right

I modified the logic and added a mulHeader field.

Can you add a comment like this to these functions?

// The returned value is not safe to use after the handler returns, the request is released or other calls to PeekAll.

erikdubbelboer · 2022-10-14T10:42:41Z

header.go

@@ -1793,6 +1793,69 @@ func (h *RequestHeader) peek(key []byte) []byte {
 	}
 }

+// PeekAll returns header value for the given key.
+func (h *RequestHeader) PeekAll(key string) [][]byte {
+	k := getHeaderKeyBytes(&h.bufKV, key, h.disableNormalizing)


Can you add a comment like this to these functions?

// The returned value is not safe to use after the handler returns, the request is released or other calls to PeekAll.

li-jin-gou · 2022-10-14T12:06:05Z

Comments have been modified.

erikdubbelboer · 2022-10-15T12:22:05Z

args.go

+func peekAllArgBytesToDst(dst *[][]byte, h []argsKV, k []byte) {
+	for i, n := 0, len(h); i < n; i++ {
+		kv := &h[i]
+		if bytes.Equal(kv.key, k) {
+			*dst = append(*dst, kv.value)
+		}
+	}
+}


Sorry one minor last change. I don't think it's needed to pass a pointer to the [][]byte. Just append to it locally and return it. That's the pattern that is used in a lot of other places as well. It shouldn't cause any allocations.

Suggested change

func peekAllArgBytesToDst(dst *[][]byte, h []argsKV, k []byte) {

for i, n := 0, len(h); i < n; i++ {

kv := &h[i]

if bytes.Equal(kv.key, k) {

*dst = append(*dst, kv.value)

}

}

}

func peekAllArgBytesToDst(dst [][]byte, h []argsKV, k []byte) [][]byte {

for i, n := 0, len(h); i < n; i++ {

kv := &h[i]

if bytes.Equal(kv.key, k) {

dst = append(dst, kv.value)

}

}

return dst

}

Thanks and done

erikdubbelboer · 2022-10-15T13:47:59Z

Thanks!

codefromthecrypt · 2022-10-21T03:48:49Z

header_test.go

+	h.Add("aaa", "aaa")
+	h.Add("aaa", "bbb")
+
+	expectResponseHeaderAll(t, h, HeaderContentType, [][]byte{s2b("aaa/bbb")})


does it make sense to add a test for when content type is deleted, whether the expected result is the default type or empty? see #1402 as it is ambiguous.

Thanks for reminding

erikdubbelboer requested changes Oct 12, 2022

View reviewed changes

li-jin-gou force-pushed the feat/add_header_peek_all branch 5 times, most recently from 120b7ec to acabcd0 Compare October 13, 2022 05:07

erikdubbelboer requested changes Oct 14, 2022

View reviewed changes

li-jin-gou force-pushed the feat/add_header_peek_all branch from acabcd0 to 5ca50dd Compare October 14, 2022 12:03

erikdubbelboer requested changes Oct 15, 2022

View reviewed changes

feat: add header.PeekAll

edd8635

li-jin-gou force-pushed the feat/add_header_peek_all branch from 5ca50dd to edd8635 Compare October 15, 2022 12:53

erikdubbelboer merged commit 2c8ce3b into valyala:master Oct 15, 2022

codefromthecrypt mentioned this pull request Oct 20, 2022

Add more functions to ease use of trailing headers #1401

Open

codefromthecrypt reviewed Oct 21, 2022

View reviewed changes

li-jin-gou mentioned this pull request Oct 21, 2022

optimize: adjust the behavior of PeekAll based on VisitAll #1403

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add header.PeekAll #1394

feat: add header.PeekAll #1394

li-jin-gou commented Oct 12, 2022 •

edited

erikdubbelboer Oct 12, 2022

li-jin-gou Oct 13, 2022

erikdubbelboer Oct 12, 2022

li-jin-gou Oct 13, 2022

erikdubbelboer Oct 12, 2022

li-jin-gou Oct 13, 2022

erikdubbelboer Oct 12, 2022

li-jin-gou Oct 13, 2022

li-jin-gou Oct 13, 2022 •

edited

erikdubbelboer Oct 14, 2022

li-jin-gou Oct 14, 2022

erikdubbelboer Oct 14, 2022

li-jin-gou commented Oct 14, 2022

erikdubbelboer Oct 15, 2022

li-jin-gou Oct 15, 2022

erikdubbelboer commented Oct 15, 2022

codefromthecrypt Oct 21, 2022

li-jin-gou Oct 21, 2022

feat: add header.PeekAll #1394

feat: add header.PeekAll #1394

Conversation

li-jin-gou commented Oct 12, 2022 • edited

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

li-jin-gou Oct 13, 2022 • edited

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

li-jin-gou commented Oct 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdubbelboer commented Oct 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

li-jin-gou commented Oct 12, 2022 •

edited

li-jin-gou Oct 13, 2022 •

edited