Added simple caching and set the default to use source importer #51
Conversation
internal/gbimporter/gbimporter.go
Outdated
| i.cache[path] = pkg | ||
| // delete the pakcage after ttl expires | ||
| timer := time.NewTimer(time.Duration(i.ttl) * time.Minute) | ||
| go func() { |
There was a problem hiding this comment.
Does it really make sense to spawn a new goroutine for each and every imported package that lives for 60 minutes?
How about including the time the package has been added to the cache and check if it's still in the ttl
(sample code, probably not 100% correct)
if c, ok := i.cache[path]; ok {
if time.Since(c.Added) < time.Duration(i.ttl)*time.Minute {
delete(i.cache, path)
} else {
return c.Pkg, nil
}
}
// .....
i.cache[path] = cachedPackage{
Added: time.Now(),
Pkg: pkg,
}There was a problem hiding this comment.
Good idea removed the go routines and store the ttl as a unix time and compare it
internal/gbimporter/gbimporter.go
Outdated
| @@ -63,6 +68,11 @@ func (i *importer) ImportFrom(path, srcDir string, mode types.ImportMode) (*type | |||
| buildDefaultLock.Lock() | |||
| defer buildDefaultLock.Unlock() | |||
|
|
|||
| // return the package if it's in the cache | |||
| if i.cache[path] != nil { | |||
There was a problem hiding this comment.
the old gocode had a check on the last modification date of the file to avoid unnecessary work:
https://github.com/nsf/gocode/blob/7b1d4e18cdc58a74dc1bd4c2d45b3f1b2ca227c3/declcache.go#L75-L91
Have you checked if something like this can be implemented here too?
There was a problem hiding this comment.
The old gocode parsed a compiled pkg file. This doesn't know about the compiled package file to do a stat on.
server.go
Outdated
| @@ -63,6 +65,7 @@ type AutoCompleteRequest struct { | |||
| type AutoCompleteReply struct { | |||
| Candidates []suggest.Candidate | |||
| Len int | |||
| Time time.Time | |||
There was a problem hiding this comment.
I cannot see that this is used anywhere...
There was a problem hiding this comment.
Yep removed in latest merge.
|
@lloiser @anjmao can I just flag #46 (comment) |
…ompare, added clearcache client command, and no caching if ttl == 0
|
The gimme script in the Travis build is not pulling 1.11 when master is referenced, instead it's pulling: go version devel +67ac554d79 Mon Sep 3 16:46:59 2018 +0000 linux/amd64 |
…ys stay cached in memery if no request are made to the daemon
Merging ticker to expire cached packages
|
There is already a branch that adds caching, and I think it's pretty similar to what you have here. I can investigate merging the branch into master, but I don't think we will accept this since it's too similar. Sorry about that! |
|
@stamblerre I looked at the cache branch and it was 2 months behind master. Do you know when you will be merging this capability into master? |
|
@phenixrizen I'll try to take a look at it today, but I'm not too sure yet. We are just maintaining gocode now, so I'm not sure if we will integrate any significant changes right now. |
Added a simple map to use as a cache for package lookups, added ttl on the packages cached by using go routine with a timer to expire the package in the map after 60 min. This default ttl can be adjusted with the cachettl flag. Also added the ability to profile gocode by passing the profile flag.