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

x/tools/go/packages: include directness information to ParseFile callback #28740

Open
zombiezen opened this issue Nov 12, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@zombiezen
Copy link
Member

commented Nov 12, 2018

Wire recently switched to go/packages after using golang.org/x/tools/go/loader. As noted in google/go-cloud#663, we observed a ~4x slowdown. Since not all of the time is spent inside the Wire process, profiling has been difficult. However, for a baseline Wire run that took 2.26s (originally 0.3s), half the time is spent in go list, the other half is spent in go/types.dependencyGraph and map assignments.

This indicates to me that at least part of the problem still is having too much input being sent to the typechecker. One of the tricks Wire employed in the loader-based code was to skip function bodies for dependency packages (Wire needs function body type-checking for the root packages). However, the ParseFile callback does not give enough information to make this determination. I would like for the arguments to ParseFile to include whether the file is being parsed for the root packages or imported packages. Something like:

package packages

type Config struct {
  // ...
  ParseFile func(fset *token.FileSet, filename string, src []byte, isRoot bool) (*ast.File, error)
}

It does seem quite possible that more information could be added over time, so a more future-friendly change could be along the lines of:

package packages

type ParseFileRequest struct {
  Fset *token.FileSet
  Filename string
  Src []byte
  IsRoot bool
}

type Config struct {
  // ...
  ParseFile func(*ParseFileRequest) (*ast.File, error)
}

@gopherbot gopherbot added this to the Unreleased milestone Nov 12, 2018

@zombiezen

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2018

/cc @alandonovan @ianthehat @matloob

Wire is receiving user reports of bad performance due to this issue (see google/go-cloud#702). Any chance we can get this in before the API freeze?

@matloob

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

I'm pretty sure that we don't want to have something along the lines of a ParseFileRequest. We don't want there to be too many knobs in the go/packages API.

One alternative idea I have is to expose the "Refine" function. Refine is called after we get the results from the driver, which is more or less what the LoadFiles mode returns. It does the rest of the work of go/packages for the rest of the modes: parsing, typechecking, etc. If we can expose Refine, you can get the results of a LoadFiles query, save the set of files in the roots packages in a side table, and then call Refine with a ParseFile method that will behave differently depending on the results of looking up the filename in the set.

Did my explanation of Refine make sense? And is that something you'd be able to use to solve this?

@zombiezen

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2018

I partially understand, but I'm unclear as to what I as a caller could do with the Refine results. I take it I would call Refine instead of Load, but how do I get from that result to a *types.Package? It would be really nice if I could do whatever operation is there multiple times to get different modes for different parts of the graph as needed. (Wire seldom needs ASTs for the entire transitive closure, usually just types, but it doesn't know ahead of time which parts it needs.)

@matloob

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

Sorry for the super late response:

Refine would return a fully-filled (as Load would) packages.Package, that you can extract the type information from. And running refine in different modes would allow you to get the graph processed in different modes. Though unfortunately, I don't think you'd be able to avoid currently getting the ASTs for the entire transitive closure.

Perhaps we'll have a better story for this once we "refactor" the load mode setup: See my comment here: #29429 (comment) and Ian/Jay's responses

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.