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

various performance optimizations #926

Closed
wants to merge 1 commit into from

Conversation

vikstrous
Copy link
Contributor

This is not a PR that's meant to be merged. It's just a POC of some performance optimizations discussed in #918

@vikstrous vikstrous mentioned this pull request Nov 10, 2019
@vektah
Copy link
Collaborator

vektah commented Nov 10, 2019

Nice exploration, good to see someone dig in here.

Sadly, I dont think its going to be this straightforward. The reason for multiple packages.Load calls is plugins change the code on disk for a package (eg modelgen).

If we want to go to a single pass we need a way for plugins to invalidate and refetch some packages, this didnt exist in packages when the code was written but maybe it does now.

One of the ideas we threw around at the time was using one of the long running language servers, which should handle reloading packages better - eg gopls or guru. But we would need to be careful that go generate can still run cleanly inside a docker container where inotify events can be quite when using docker for mac and volume mounts.

@@ -92,24 +90,39 @@ func ImportPathForDir(dir string) (res string) {

var modregex = regexp.MustCompile("module (.*)\n")

type NameForPackage struct {
cache *sync.Map
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this need to be moved out of a private global?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically it didn't need to be moved, but it was weird to me to use global state when we were already creating a struct to hold the packages field.

In addition to the fact that global state is a dangerous anti-pattern, I suspect that this would also have caused issues if I wanted to use the same binary to generate multiple graphql servers.

@vektah
Copy link
Collaborator

vektah commented Nov 28, 2019

This has been split out into
#940
#941
#942
#943
#944
#945

@vektah vektah closed this Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants