Large models fail to update #41
Conversation
…r/value pair already created. Changes have been done in such a way they shouldn't affect the other uses of the code. Unfortunately there are no unit tests included to ensure this takes place, but unit tests broken by the changes have been fixed which will confirm parameter reuse. This is an attempt to fix http://entityframework.codeplex.com/workitem/520
|
Hi @bengutt, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
|
@bengutt, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
|
Some issue with line endings or similar makes the diff unreadable.. |
|
Somehow I managed to replace LF with CRLF. |
|
Oh...I'm excited to see progress on this item. We were hit by this issue a while back and I would love to get rid of our workarounds. Thanks @bengutt!! |
|
👍 @bengutt |
|
Any news on this front? I suspect I'm about to hit this limit again :( |
| @@ -292,6 +292,50 @@ private static int IndexOf(IEnumerable items, string parameterName) | |||
| return -1; | |||
| } | |||
|
|
|||
| /// <inhertidoc /> | |||
There was a problem hiding this comment.
Probably right, the others already in this class were the same, so I stuck with it.
| return InnerList[index]; | ||
| } | ||
|
|
||
| private static int IndexOfParameterValue(IEnumerable items, string parameterValue) |
There was a problem hiding this comment.
Couldn't we keep a map of parameterValue to index? Then looking up the index would be O(1)?
There was a problem hiding this comment.
Possibly, I used the methed "private static int IndexOf(IEnumerable items, string parameterName)" in the same class as the template. Using a dictionary(?) for a look up would only give you the first compare. You'd end up having to do the second lookup and null checks anyway, so the code would look almost identical, with the exception of having an additional dictionary to manage.
I guess you could alter the InnerList property to return the values collection but would that give you anything worth the scale of the change?
There was a problem hiding this comment.
@bengutt missed this before, but can you elaborate on why both comparisons are needed?
There was a problem hiding this comment.
It's down to globalisation.
The first comparison does a standard does stringA match stringB, nice efficient and quick. The second does a match based on the Culture which is slower, and will be needed by far fewer clients.
My understanding is that in the majority of cases the first condition is adequate. I think the reason for the second check was mainly around the StringCompareOption of IgnoreKanaType. Rather than me attempting to explain, best bet is to view the MSDN article on it - https://msdn.microsoft.com/en-us/library/system.data.sqltypes.sqlstring.ignorekanatype(v=vs.110).aspx
There was a problem hiding this comment.
@bengutt would you say the first check will already succeed if the parameters values are identical? I think the goal of this fix should be to take advantage of the fact that many of the parameters generated for the query are actually identical, i.e. the exact same table names are passed in multiple (AFAIR at least 3) parameters, plus the schema is typically always the same or has very few different values.
Unless I am missing something, matching values that are not identical but similar (i.e. that only satisfy the second check but not the first one) only provides marginal value.
I am not super concerned for the performance of this but I suspect the fix could be simpler.
There was a problem hiding this comment.
I'd be happy that the fix would still be effective if the second check was removed. In the vast majority of cases the values would be identical or not match at all.
Now thinking about it from a different angle, it's probably more efficient to drop the second compare. At best it will only provide a match a handful of times, yet will run through the collection a second time for every new parameter. I was so focussed on reducing the number of parameters it was generating I missed this.
I'd agree that the second check isn't warranted, created noise, and should be removed. Thanks for spotting it.
There was a problem hiding this comment.
@bengutt thanks to you for contributing this fix!
I will go ahead and provide some additional feedback on this PR although it is already merged. Depending on the timing we can take an additional PR from you or do some simplifications on top of what you did.
|
Slightly OT, sorry, but I'd really like to try out this PR on my machine. |
|
I had to build it in 2013, and even then it was a little dance to get it to work. We have used the resulting installer in 2013 and 2015 (there is a different output for each one) for a number of years now. I'd be tempted to go down the VM route rather than installing 2013 and messing with security on your machine. I've not tried to get it working (building or running) in vs2017 so can't help with that I'm afraid. |
|
Great news, looking forward to the next release so we can finally be on the official code! |
|
@bengutt You're welcome - thank you for the contribution. And sorry it took me a while to validate - we've had some problems getting builds working too ;-) |
| @@ -149,15 +149,24 @@ internal StringBuilder CreateWhereClause(EntityParameterCollection parameters) | |||
|
|
|||
| if (value != null) | |||
| { | |||
| var parameterName = "p" + parameters.Count.ToString(CultureInfo.InvariantCulture); | |||
| var matchingParameter = parameters.GetParameterByValue(value); | |||
There was a problem hiding this comment.
If we simplify the matching logic to just do one equality check then I think it makes sense to revisit @lajones's original suggestion to use a dictionary and some GetOrAdd-like logic here.
| [SuppressMessage("StyleCop.CSharp.DocumentationRules", "SA1611:ElementParametersMustBeDocumented")] | ||
| [SuppressMessage("StyleCop.CSharp.DocumentationRules", "SA1615:ElementReturnValueMustBeDocumented")] | ||
| [SuppressMessage("Microsoft.Usage", "CA2201:DoNotRaiseReservedExceptionTypes")] | ||
| public DbParameter GetParameterByValue(string value) |
There was a problem hiding this comment.
This adds new public surface to the runtime for a scenario that seems very narrow, e.g. it assumes the string representation of the value is relevant, and turns doing a reverse lookup of parameters based on this string representation a general feature. I believe we should be able to refactor so that the workaround is completely implemented outside of this class by keeping track of the mapping between values and already added parameter instances in a dictionary.
| } | ||
| } | ||
|
|
||
| // <summary> | ||
| // Gets the ToString() representation of the value of this parameter. | ||
| // </summary> | ||
| internal string ParameterStringValue |
There was a problem hiding this comment.
Similar to a previous comment, this new field and parameter add public surface and overhead that doesn't seem to be used outside of the narrow scenario we are trying to fix. If we track a map of values to parameter instances while creating the schema queries (where we know that all parameter values are string) we shouldn't need this.
|
Just advising that I was able to fetch the branch from master that had this PR merged (tagged as 6.2 alpha) and build it under the VS 2015 developer command prompt. The resulting MSI didn't work (install says "a newer product is already installed", but repair and uninstall are "only valid for products that are installed"). However I could take the various built assemblies and overwrite them in the VS 2015 Common\IDE folder and then VS 2015 was able to update my model. I wasn't game to muck with my VS 2017 install :) Thanks very much for this update 🎉 I'm looking forward to the official release. |
|
Hi, You need to uninstall the existing copies of entry framework via add/remove programs before doing the install. One trap we fall into to is, after a patch or repair, a developer box installs the official release and uninstalls the one with the large model fix.... I'm hoping to have a go at improving the fix, with the suggestions here and providing a new PR next week or the week after. We've an absolutely massive model so I'll be in a position to give it a real test. EDIT: I've just spotted the work has been done already in #185, it's currently in the Master branch. I'll be grabbing a copy of that and running with that until the official release. |
…into tools code Supersedes #41 with a very similar approach (avoiding creating duplicate parameters with the same values) but the implementation in this case lives completely into the EF Tools code and avoids changes to the EF Runtime.
This is a best attempt fix for http://entityframework.codeplex.com/workitem/520. The problem has the potential to reoccur due to the fact that there is a fixed limit of 2100 parameters in SQL server. A rewrite other the entire function would need to be completed in order to get round it . You will need a very, very, large model to hit the problem again with this fix in place.
This issues occurs when you have a large number of tables in a model and you need to update the model.
When the parameters are being generated for the update, the code is now checking that there isn't a parameter already created with the same value. If it has it will reuse that parameter instead of simply just adding another one.
Additional information is available in the Issue.
While there are no unit tests specifically for this fix, it has been used extensively in house without issue. Further existing unit tests had to be altered in order to pass to allow for the parameter reuse.