Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add header.PeekAll #1394

Merged
merged 1 commit into from Oct 15, 2022

Conversation

li-jin-gou
Copy link
Contributor

@li-jin-gou li-jin-gou commented Oct 12, 2022

pr for #179

args.go Outdated
@@ -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())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

args.go Outdated
@@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

args.go Outdated
@@ -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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks,You're right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified the logic and added a mulHeader field.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@li-jin-gou 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
@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Comments have been modified.

args.go Outdated
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)
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks and done

@erikdubbelboer erikdubbelboer merged commit 2c8ce3b into valyala:master Oct 15, 2022
14 checks passed
@erikdubbelboer
Copy link
Collaborator

Thanks!

h.Add("aaa", "aaa")
h.Add("aaa", "bbb")

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reminding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants