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

time: LoadLocation is slow #26106

Closed
zhexuany opened this issue Jun 28, 2018 · 7 comments
Closed

time: LoadLocation is slow #26106

zhexuany opened this issue Jun 28, 2018 · 7 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

@zhexuany
Copy link

Please answer these questions before submitting your issue. Thanks!

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

1.10

Does this issue reproduce with the latest release?

Just benchmark time.LocadLocation.

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/zhexuany/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/zhexuany/repo/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.10.3/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.10.3/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/t9/s5qvdzpx3h5g58xzpm5gn8l00000gn/T/go-build785631011=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Just benchmark LoadLocation, you will see the result.

What did you expect to see?

Each LoadLocation should be finished in a very reasonable time.

What did you see instead?

Each LoadLocation takes about 30us. It is huge.

@ALTree Here is the issue.
If go can do some kinda cache of location, that will be better. Right now, LoadLocation is relatively slow.

@ALTree ALTree changed the title time: LoadLocation is slow. time: LoadLocation is slow Jun 28, 2018
@ALTree ALTree added this to the Unplanned milestone Jun 28, 2018
@cznic
Copy link
Contributor

cznic commented Jun 28, 2018

Just benchmark LoadLocation, you will see the result.

Please show the numbers for your or any other machine, thanks.

@zhexuany
Copy link
Author

goos: darwin
goarch: amd64
BenchmarkTimeLoadLocation-8   	  100000	     16873 ns/op	    2916 B/op	      10 allocs/op

Above is benchmark result and snippet I used is the following

package main
import (
	"archive/zip"
	"log"
	"math/rand"
	"runtime"
	"testing"
	"time"
)

func init() {
	rand.Seed(time.Now().UnixNano())
}

var locationNames = loadLocationNames()


func BenchmarkTimeLoadLocation(b *testing.B) {
	sample := prepareSample(b.N, locationNames)

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		time.LoadLocation(sample[i])
	}
}

func loadLocationNames() []string {
	zoneinfo := runtime.GOROOT() + "/lib/time/zoneinfo.zip"

	r, err := zip.OpenReader(zoneinfo)
	if err != nil {
		log.Fatalf("Could not open zoneinfo.zip for reading: path '%s' error: '%v'", zoneinfo, err)
	}
	defer r.Close()

	var locs = []string{}

	for _, f := range r.File {
		if f == nil {
			log.Fatalf("zoneinfo.zip contained nil file pointer")
		}

		if f.FileHeader.FileInfo().IsDir() {
			continue
		}

		locs = append(locs, f.Name)
	}
	return locs
}

func prepareSample(n int, data []string) []string {
	dn := len(data)
	sample := make([]string, n)
	for i := 0; i < n; i++ {
		sample[i] = data[rand.Intn(dn)]
	}

	return sample
}

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 28, 2018
@meirf
Copy link
Contributor

meirf commented Jun 29, 2018

$ go test -bench=BenchmarkTimeLoadLocation -benchmem -cpuprofile=cpu.out
$ go tool pprof cpu.out
...
(pprof) top10 -cum
...
      flat  flat%   sum%        cum   cum%
         0     0%     0%      1.65s 73.33%  time.readFile
     1.62s 72.00% 72.00%      1.62s 72.00%  syscall.Syscall
         0     0% 72.00%      1.61s 71.56%  testing.(*B).launch
         0     0% 72.00%      1.61s 71.56%  testing.(*B).runN
         0     0% 72.00%      1.61s 71.56%  time.LoadLocation
         0     0% 72.00%      1.61s 71.56%  time.loadLocation
         0     0% 72.00%      1.61s 71.56%  time.loadTzinfo
         0     0% 72.00%      1.61s 71.56%  time.loadTzinfoFromDirOrZip
         0     0% 72.00%      1.61s 71.56%  time_test.BenchmarkTimeLoadLocation
         0     0% 72.00%      1.28s 56.89%  syscall.Read

This is expected based on LoadLocation's godoc (multiple mentions of file reading). Further, even if a user doesn't read the godoc, "Load" in the function name should make the disk reading unsurprising. So I think we can agree that some slowness should be expected.

If go can do some kinda cache of location, that will be better. Right now, LoadLocation is relatively slow.

I wonder if there is a precedent in the standard library for caching something like this. If not, maybe caching should be left to the user of the library since anything short of caching everything (which we definitely don't want in the std lib) is highly dependent on usage patterns: how many entries should be cached? should the number of entries in the cache be configurable? should there be an eviction policy or should items be cached forever? etc.

@ianlancetaylor
Copy link
Contributor

The docs for time.LoadLocation also make clear that it looks in the file system. If that is the source of the slowness then there is nothing we can do.

I agree that LoadLocation itself should not do any caching. It's easy to wrap a cache around it if that is appropriate for your application.

@agnivade
Copy link
Contributor

I agree that LoadLocation itself should not do any caching.

Well, that is what this issue is about. For reference, this came from here - #24844 (comment). @zhexuany is aware that time.LoadLocation looks in the filesystem, and wants the results to be cached in the standard library itself.

If the decision is that LoadLocation should not do any caching, then I believe this can be closed as working as intended.

@zhexuany
Copy link
Author

zhexuany commented Jun 29, 2018

Yes. Our solution is to wrap time.LoadLocation into a cache policy which improves the performance. The easiest cache policy is to record appeared time.Location into a map in current instance whose key is the loc.String(). Is it more easier adding cache policy into standard lib and let user decides which one to use? It is just my naive thought. Any comments are welcome.

@ianlancetaylor
Copy link
Contributor

Caching time.LoadLocation in the standard library would be an unfortunate choice for programs that want to open a bunch of locations once. Only the program can possibly know the appropriate caching to use for locations. Adding a bunch of caching policies into the standard library significantly complicates the API and the implementation for a very small number of programs, and we would probably still miss important use cases. Basically, caching is simple for the program and very hard, perhaps impossible, to do satisfactorily in the standard library. Closing this issue as working as expected.

@golang golang locked and limited conversation to collaborators Jun 29, 2019
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

7 participants