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

Fixed performance issue caused by loading config multiple times #133

Merged
merged 1 commit into from
Mar 31, 2017

Conversation

tamccall
Copy link
Contributor

@tamccall tamccall commented Mar 27, 2017

Fixed performance issues caused by loading config after every file load

See attached cpu profiles for more info on performance issues.
Measured cpu profile on make integration in both cases
cpu-profile.zip

Also fixes #104

@codeactual
Copy link
Contributor

I think there are several opportunities to improve this for review-ability and scope.

  • Remove as many file changes as possible which are not critical to the PR. GitHub says that 1,889 files were changed and 823,428 lines were added. A quick look at the manifest shows things like vendor tests, JS, and other non-Go code files.
  • Consider splitting the -cpuprofile feature into a separate PR from the performance fix. I see how they were related in origin but they're easier to review apart.
  • Package management seems endlessly controversial. I've used glide, worked well for me. But adoption of it might also fit better in its own PR that is only about packaging.

@tamccall
Copy link
Contributor Author

Fair enough I'll just get the relevant changes in here.

But FWIW vendoring is something that should seriously be considered

@tamccall
Copy link
Contributor Author

Added cpuprofile cli option PR #134

@tamccall
Copy link
Contributor Author

Added issue to discuss preferred vendoring tools #135

@evanphx evanphx merged commit c90a42f into vektra:master Mar 31, 2017
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.

Files generated multiple times
3 participants