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

Comments

Projects
None yet
6 participants
@zhexuany
Copy link

commented Jun 28, 2018

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 the Performance label Jun 28, 2018

@ALTree ALTree added this to the Unplanned milestone Jun 28, 2018

@cznic

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2018

Just benchmark LoadLocation, you will see the result.

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

@zhexuany

This comment has been minimized.

Copy link
Author

commented Jun 28, 2018

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
}
@meirf

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

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

This comment has been minimized.

Copy link
Member

commented Jun 29, 2018

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.