Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Optimize SqlClient rpc parameter usage #34049

Merged
merged 13 commits into from
Apr 4, 2019
Merged

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Dec 12, 2018

I have made changes to the _SqlRPC object which change how the system and user parameters are allocated and stored. Originally the system parameters were allocated each execution, I've altered this so the correct number are allocated as needed and re-used cleanly if the query type changes (between prepare, unprepared, proc and exec).
User parameters are no longer copied in but referenced by index into the parameter collection.

This is a risky change. It needs careful review to ensure compatibility.

This PR was split from #32811 and into smaller commits for easier review. It has passed the manual and efcore tests in native mode, the tests cannot be successfully run in managed mode due to #33930 . Performance improvements for this and related PR's are in the original discussion.
/cc @tarikulsabbir, @afsanehr @saurabh500 @David-Engel

@karelz karelz added this to the 3.0 milestone Dec 21, 2018
@Wraith2
Copy link
Contributor Author

Wraith2 commented Feb 1, 2019

Merge conflict resolved. This has been open since October last year on the original PR, any chance of getting some feedback?

@AfsanehR-zz
Copy link
Contributor

Thanks for resolving the conflicts @Wraith2 . We will start looking into this pr soon.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Feb 20, 2019

How soon?

@Wraith2
Copy link
Contributor Author

Wraith2 commented Mar 3, 2019

This PR is on page 3, of 3 in the list. It's been open almost 3 months now with no review, it was open for several months before that on the original PR which was deemed too complex. Can I get some feedback or a decision that this change just isn't going to be approved? I keep running into the performance issues this addresses in other areas of the project and having to ignore them knowing I've already fixed it.
@karelz who do I need to bother to get time allocated?

@karelz
Copy link
Member

karelz commented Mar 4, 2019

I am guilty of not monitoring & pushing on stale PRs for a while (like 4+ weeks :(), sorry for that.
@afsanehr @tarikulsabbir @saurabh500 @Gary-Zh @David-Engel do you have any thoughts on this PR? Feel free to ping me on email if answer is more complicated - I can help! (saying no is also ok with explanation of reasons)

@karelz
Copy link
Member

karelz commented Mar 18, 2019

@afsanehr @tarikulsabbir @Gary-Zh any update?

@tarikulsabbir
Copy link
Contributor

Hi, Sorry for the delay. I have started looking into this PR.

@karelz
Copy link
Member

karelz commented Apr 1, 2019

@tarikulsabbir @Wraith2 is it blocked only on passing tests?

@Wraith2
Copy link
Contributor Author

Wraith2 commented Apr 1, 2019

As far as I know, just merged to master which will re-run the CI.

@tarikulsabbir
Copy link
Contributor

@tarikulsabbir @Wraith2 is it blocked only on passing tests?

Hi @karelz and @Wraith2 , we are currently running EFCore tests on different platforms. We had a few issues on our test environment which is now resolved. Will update again soon.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Apr 1, 2019

CI failures are valuetuple and linq, unrelated to this change.

@tarikulsabbir
Copy link
Contributor

The tests are green. There were a few environmental issues which caused the delay. Anyway I will merge the PR now. Thanks for your patience.

@tarikulsabbir tarikulsabbir merged commit 5458fe2 into dotnet:master Apr 4, 2019
@Wraith2 Wraith2 deleted the sqlperf-rpc branch April 4, 2019 23:22
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Optimize SqlClient rpc parameter usage

Commit migrated from dotnet/corefx@5458fe2
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants