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

net/url: optimization for calling URL.Query multiple times #37583

Closed
alpancs opened this issue Feb 29, 2020 · 5 comments
Closed

net/url: optimization for calling URL.Query multiple times #37583

alpancs opened this issue Feb 29, 2020 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@alpancs
Copy link

alpancs commented Feb 29, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/alfan/.cache/go-build"
GOENV="/home/alfan/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/alfan/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/alfan/Documents/bukalapak/wagyu/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build085565713=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package main

import (
	"fmt"
	"net/url"
	"time"
)

func main() {
	someURL, _ := url.Parse("https://example.com?a=a&b=b&c=c&d=d&e=e&f=f&g=g&h=h&i=i&j=j") // error omitted

	startTime := time.Now()
	for i := 0; i < 500111; i++ {
		someURL.Query()
	}

	fmt.Println("elapsed time:", time.Since(startTime))
}

*I write the source code here instead of writing it in play.golang.org because on that environment the output was always elapsed time: 0s (no idea 🤷‍♂️️)

What did you expect to see?

elapsed time: 325.914µs
(or something under 1ms)

What did you see instead?

elapsed time: 856.723768ms
@alpancs
Copy link
Author

alpancs commented Feb 29, 2020

I'm planning to add a cache for the return value of method URL.Query.

  1. Add two private fields of type URL
type URL struct {
	Scheme     string
	Opaque     string    // encoded opaque data
	User       *Userinfo // username and password information
	Host       string    // host or host:port
	Path       string    // path (relative paths may omit leading slash)
	RawPath    string    // encoded path hint (see EscapedPath method)
	ForceQuery bool      // append a query ('?') even if RawQuery is empty
	RawQuery   string    // encoded query values, without '?'
	Fragment   string    // fragment for references, without '#'

	lastRawQueryParsed string
	lastQueryResult Values
}
  1. Use lastQueryResult as return value of method URL.Query when lastRawQueryParsed = RawQuery; otherwise get the return value first, then update the value of lastRawQueryParsed and lastQueryResult

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 29, 2020
@dmitshur dmitshur added this to the Backlog milestone Feb 29, 2020
@dmitshur
Copy link
Contributor

Thanks for filing an issue and discussing a potential fix first.

The url.URL type is very low level and used by many Go programs. The benefit of this optimization will need to be evaluated against the cost very carefully.

It may not be viable to add unexported fields to the url.URL type, because it'll have significant effects on the usability of this type. For example, it won't be possible to compare two url.URL values with == operator reliably.

To have good performance, users can implement caching in their handlers if they need to use the result of Query method often:

q := req.URL.Query()
// ... use q many times

Alternatively, http.Request.FormValue already implements caching and can be used instead:

// use req.FormValue() many times

/cc @ianlancetaylor @dsnet

@dmitshur
Copy link
Contributor

*I write the source code here instead of writing it in play.golang.org because on that environment the output was always elapsed time: 0s (no idea 🤷‍♂️️)

This is expected behavior. Computations are instantaneous on the Go playground and don't use up time. Read the Faking time section of the playground blog post.

@alpancs
Copy link
Author

alpancs commented Mar 1, 2020

Thanks for the advice @dmitshur.
I think the problem has been solved by http.Request.FormValue.

Is it OK to close this issue?

*TIL: http.Request.FormValue works for URL query. 😅

@dmitshur
Copy link
Contributor

dmitshur commented Mar 1, 2020

Glad to hear it!

Let's wait a few days to see if anyone else has more thoughts on this, but after that it should be fine to close.

@alpancs alpancs closed this as completed Mar 7, 2020
@golang golang locked and limited conversation to collaborators Mar 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

3 participants