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

go/doc: if New weren't destructively, cmd/api would be faster #4380

Closed
bradfitz opened this issue Nov 13, 2012 · 9 comments
Closed

go/doc: if New weren't destructively, cmd/api would be faster #4380

bradfitz opened this issue Nov 13, 2012 · 9 comments

Comments

@bradfitz
Copy link
Contributor

go/doc.New is destructive:

  http://golang.org/pkg/go/doc/#New

It it weren't, cmd/api could avoiding many calls to ParseFile and run much quicker than
10 seconds (and will be getting worse, as we add more operating systems for go 1.1)
@bradfitz
Copy link
Contributor Author

Comment 1:

Here's what needs to be done to goapi to make it faster, once this is fixed:
diff -r 821585f8baba src/cmd/api/goapi.go
--- a/src/cmd/api/goapi.go      Fri Nov 16 11:53:26 2012 -0800
+++ b/src/cmd/api/goapi.go      Fri Nov 16 13:15:51 2012 -0800
@@ -353,6 +353,15 @@
        return
 }
 
+var parsedFileCache = make(map[string]*ast.File)
+
+func parseFile(filename string) (*ast.File, error) {
+       if f, ok := parsedFileCache[filename]; ok {
+               return f, nil
+       }
+       return parser.ParseFile(fset, filename, nil, 0)
+}
+
 // WalkPackage walks all files in package `name'.
 // WalkPackage does nothing if the package has already been loaded.
 func (w *Walker) WalkPackage(name string) {
@@ -386,7 +395,8 @@
 
        files := append(append([]string{}, info.GoFiles...), info.CgoFiles...)
        for _, file := range files {
-               f, err := parser.ParseFile(fset, filepath.Join(dir, file), nil, 0)
+               filename := filepath.Join(dir, file)
+               f, err := parseFile(filename)
                if err != nil {
                        log.Fatalf("error parsing package %s, file %s: %v", name, file, err)
                }
@@ -395,6 +405,8 @@
                for _, dep := range fileDeps(f) {
                        w.WalkPackage(dep)
                }
+               parsedFileCache[filename] = f
+
        }
 
        if *verbose {

@bradfitz
Copy link
Contributor Author

Comment 2:

(or actually parseFile can populate the cache itself.  I tried moving it out to where it
is now when debugging the problem, before I remembered this issue...)

@bradfitz
Copy link
Contributor Author

Comment 3:

This issue was updated by revision aeca7a7.

R=golang-dev, remyoudompheng, minux.ma, gri
CC=golang-dev
http://golang.org/cl/6845058

@rsc
Copy link
Contributor

rsc commented Dec 30, 2012

Comment 4:

Labels changed: added priority-later, removed priority-triage.

@robpike
Copy link
Contributor

robpike commented Mar 7, 2013

Comment 5:

Labels changed: removed go1.1maybe.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 6:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 7:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 8:

Labels changed: added repo-main.

@griesemer
Copy link
Contributor

Comment 9:

go/doc.New is still destructive but goapi was rewritten to use go/types instead.
Closing this since it doesn't affect goapi anymore.

Status changed to Fixed.

@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants