-
Notifications
You must be signed in to change notification settings - Fork 178
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
Improve performance of .NET Core Analyze #296
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR LGTM. Thanks for the contribution! We'll merge once we get the tests passing (this looks like CI misconfiguration on our end, we'll take care of it).
@zlav can you take a look at the CI failures here? They're related to the coverage statistics we added.
It looks like we might want to do something similar to cloudflare/cfssl#203.
I'm a bit concerned that panics and To improve the performance of this, we should probably have a |
ddf1c7b
to
31d62f3
Compare
@liftM I put the sync back in, it provided no perf benefit when the log message in question was removed. |
31d62f3
to
e03ee77
Compare
result in a 2GB log file, and will be extremely slow.
e03ee77
to
fc4d0c7
Compare
Merging despite end-to-end test failure so I can cut a release. @lackstein: this end-to-end test failure is because push-only keys still do not work correctly, so we're unable to do tests with API keys for public forks. What's the status on that? |
Calling file.Sync() for each log message has an extremely negative performance impact. Additionally, logging each Package/Project reference in a mid-sized solution will result in a 2GB log file.
There's likely some additional perf gains to be had in this analyze implementation, but this is the low-hanging fruit.