-
Notifications
You must be signed in to change notification settings - Fork 32
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
Synchronous reads for improved performance for large payloads #134
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.
Thanks for this PR! I'd like to reduce its scope a little so that it only impacts execution-related SQL calls. I'd also like it if we can try to reduce some of the duplication.
4d27541
to
3d0ddbb
Compare
@cgillum a new version is coming as my fix was incorrect the performance issue what on the reader and not on ExecuteReaderAsync I let a first run to show the test performance (on my computer it was around 1 min 40 and after the fix it was 12sec) |
6479b38
to
1bb76f4
Compare
@bhugot I just verified your fix on my machine: your stress test takes 1.1 minutes without the fix and is 4.5 seconds with your fix. That's pretty incredible! |
:) |
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.
@bhugot thanks for this great PR! I added some minor changes to clean up a few things. Let me know if you have any questions or concerns about my changes.
It's ok for me |
…on because of mssql client async bug sqlcli
It should be easy to switch back when the fix is done
It should also fix #126