cache godoc package data #70

vmpjdc · 2019-10-23T02:12:33Z

Now that gopkg.in is hosted in the UK, godoc requests made while rendering the human-readable package pages take a lot longer (often a second or more). This branch adds a cache with a TTL of 1 hour.

Also, mainly as an exercise, I've removed dataMutex and replaced gotResp with a pair of string channels.

4a6f656c

LGTM, with a couple of comments.

4a6f656c · 2019-10-23T06:37:22Z

page.go

 	}()

+	wantResps := 2


You could do this below as part of the for loop:

for r, wantResps := 0, 2; r < wantResps; {

I've implemented Gustavo's multiple-select suggestion instead.

page.go

4a6f656c · 2019-10-23T06:38:39Z

page.go

@@ -217,6 +217,43 @@ type packageData struct {
 	GitTreeName    string
 }

+type cacheEntry struct {
+	t  time.Time


I'd be inclined to call this timestamp or ts.

We can just have Timestamp as part of packageData itself and get rid of this type altogether.

I've gone with Timestamp here.

niemeyer

I don't understand the description of the change. It says it's caching package data for rendering pages, but that's already cached. What's the problem being solved in more detail?

Update: Sorry, I'm crazy. I mixed up the work I've done on the documentation proxy, which does have a nice cache, with this. Thanks for adding the cache. I'll send a review next.

niemeyer · 2019-10-23T08:42:11Z

Sorry, I'm crazy. I mixed up the work I've done on the documentation proxy, which does have a nice cache, with this. Thanks for adding the cache. I'll send a review next.

niemeyer

Idea and implementation look good. Here are a few nitpicks on the details:

niemeyer · 2019-10-23T08:31:36Z

page.go

 	r := 0
 	for r < wantResps {
 		select {
-		case <-gotResp:
+		case data.PackageName = <-name:
+			r++


Now that we have independent channels in the middle of the logic above, it's more likely we might get a bug some day about two responses being received. Instead of counting responses, let's just have something simpler and more resilient:

timeout := time.After(3 * time.Second) select { case data.PackageName := <-name: case <-timeout: } select { case data.Synopsis := <-synopsis: case <-timeout: }

Please double check, but I think this achieves the same with less logic.

Implemented, and it looks good in my testing.

niemeyer · 2019-10-23T08:33:05Z

page.go

@@ -227,7 +264,7 @@ type SearchResults struct {

 var regexpPackageName = regexp.MustCompile(`<h2 id="pkg-overview">package ([\p{L}_][\p{L}\p{Nd}_]*)</h2>`)

-func renderPackagePage(resp http.ResponseWriter, req *http.Request, repo *Repo) {
+func fetchPackageData(repo *Repo) *packageData {


That's a nice change by itself.

niemeyer · 2019-10-23T08:35:59Z

page.go

@@ -217,6 +217,43 @@ type packageData struct {
 	GitTreeName    string
 }

+type cacheEntry struct {
+	t  time.Time


We can just have Timestamp as part of packageData itself and get rid of this type altogether.

niemeyer · 2019-10-23T08:37:11Z

page.go

+}
+
+var cache map[string]*cacheEntry = make(map[string]*cacheEntry)
+var cacheLock sync.RWMutex


These are globals, so we need a more clear name for them.

Perhaps, using your theme:

pageDataCache

pageDataCacheTTL

pageDataCacheLock

Following my changes to stop caching repo, I've renamed this packageDataCache, etc.

niemeyer · 2019-10-23T08:54:08Z

page.go

+
+const cacheTTL = 1 * time.Hour
+
+func cacheGet(name string) *packageData {


Same.. we can also reverse notation so it reads like the other methods:

getPageData

setPageData
We might also optionally rename packageData to pageData, so it's all shorter and more aligned.

I've stuck with packageData, since it now only contains Name and Synopsis, but renamed the functions verb-first.

niemeyer · 2019-10-23T08:56:49Z

page.go

+var cache map[string]*cacheEntry = make(map[string]*cacheEntry)
+var cacheLock sync.RWMutex
+
+const cacheTTL = 1 * time.Hour


This will have a usability impact, in that people will push their git repository and refresh the page expecting a change. Can we start on the conservative side, caching with, say, 1 minute, and then grow from there as we learn more about usage patterns?

I've taken a look at the access logs and based on my very crude hourly histograms it looks like even the most popular package page (yaml.v2) is only visited once every five or so minutes on average, so we may not want to bother with this at all.

However, if we do: I just noticed that repo is passed in fresh every time (it's fetched by handler() in main.go) so caching it at all was pretty silly of me. We can have the best of both worlds by caching only the godoc data (name and synopsis) and using repo to calculate the versions, which should always be current. (We can always add a repoCache later with a 1-minute TTL later if we decide we'd like to speed this up further.)

I've implemented this and will push shortly.

niemeyer · 2019-10-23T08:58:58Z

page.go

+		if time.Since(entry.t) < cacheTTL {
+			return entry.pd
+		}
+		log.Println("ignoring expired cache entry for", name)


This seems a bit boring to read on the logs so frequently. It feels more like a debug message than a log one. Most accesses are not for the pages but via git, so when we get the page it'll always come with one of these and ...

niemeyer · 2019-10-23T09:00:17Z

page.go

+		if time.Since(entry.t) < cacheTTL {
+			return
+		}
+		log.Println("updating expired cache entry for", name)


... one of those.

I suggest dropping both for now.

Removed both.

Use the fresh GitHub data to calculate versions so that pushes are seen immediately.

niemeyer

Looks good, except for the TTL:

niemeyer · 2019-10-24T08:17:24Z

page.go

+var packageDataCache map[string]*packageData = make(map[string]*packageData)
+var packageDataCacheLock sync.RWMutex
+
+const packageDataCacheTTL = 1 * time.Hour


Again, can we please drop this to a much smaller time window? I suggest 1 minute at most. The issue here is usability: people pushing their own PRs will refresh this page to see it working, and it won't. If this is barely used at all, then we don't even need caching. Maybe let's drop that idea altogether then?

Actually, with that same rationale, let's do 1 minute please. This matches the other PR, and is enough to catch fast refresh action without actually making it too awkward for people finding out if their branch logic is being considered by the service.

The current version of this branch now only caches the GoDoc data, so the versions should always be fresh. It looks like GoDoc refreshes once a day (https://godoc.org/-/about) although if someone invokes a manual refresh they may notice that it no longer updates immediately.

I'll make it 1 minute and we can go from there. I'm interested in exporting metrics from the gopkg server, and cache stats would certainly be among them (actually, so far that's all I've thought of...).

4a6f656c approved these changes Oct 23, 2019

View reviewed changes

niemeyer requested changes Oct 23, 2019

View reviewed changes

vmpjdc force-pushed the package-data-cache branch 2 times, most recently from bc7ba44 to 2487c2a Compare October 24, 2019 00:22

vmpjdc added 3 commits October 24, 2019 14:19

cache package data to reduce godoc requests

daf6e9c

use one select per channel instead of counting receives

34620ab

only cache the data we fetch from godoc

cd92e43

Use the fresh GitHub data to calculate versions so that pushes are seen immediately.

vmpjdc force-pushed the package-data-cache branch from 2487c2a to cd92e43 Compare October 24, 2019 01:22

niemeyer requested changes Oct 24, 2019

View reviewed changes

set GoDoc cache TTL to 1 minute

ac6a6f7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cache godoc package data #70

cache godoc package data #70

vmpjdc commented Oct 23, 2019

4a6f656c left a comment

4a6f656c Oct 23, 2019

vmpjdc Oct 24, 2019

4a6f656c Oct 23, 2019

niemeyer Oct 23, 2019

vmpjdc Oct 24, 2019

niemeyer left a comment •

edited

Loading

niemeyer commented Oct 23, 2019

niemeyer left a comment

niemeyer Oct 23, 2019

vmpjdc Oct 24, 2019

niemeyer Oct 23, 2019

niemeyer Oct 23, 2019

niemeyer Oct 23, 2019

vmpjdc Oct 24, 2019

niemeyer Oct 23, 2019

vmpjdc Oct 24, 2019

niemeyer Oct 23, 2019

vmpjdc Oct 24, 2019

niemeyer Oct 23, 2019

niemeyer Oct 23, 2019

vmpjdc Oct 24, 2019

niemeyer left a comment

niemeyer Oct 24, 2019 •

edited

Loading

niemeyer Oct 24, 2019

vmpjdc Oct 24, 2019


		const cacheTTL = 1 * time.Hour

		func cacheGet(name string) *packageData {

cache godoc package data #70

Are you sure you want to change the base?

cache godoc package data #70

Conversation

vmpjdc commented Oct 23, 2019

4a6f656c left a 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

niemeyer left a comment • edited Loading

Choose a reason for hiding this comment

niemeyer commented Oct 23, 2019

niemeyer left a 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

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

niemeyer left a comment

Choose a reason for hiding this comment

niemeyer Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment •

edited

Loading

niemeyer Oct 24, 2019 •

edited

Loading