Query compilation performance - Part 2

[chrfin]

This is an update to #430 with the addition to be able to configure or completely disable the query optimizations as already discussed in the initial pull request.

[dnfclas]

All CLA requirements met.

[divega]

@chrfin, @ajcvickers do you know how usable is System.Diagnostics.Switch on .NET Core? From what I remember the last time I used one of these you configured in the app.config, but that isn't operational in .NET Core, so, are there other alternatives?

[chrfin]

Hmm, good point. I used a Switch because the PlanCompiler class already uses BooleanSwitch (see _applyTransformationsRegardlessOfSize)...
I'll check it and if not working on .NET Core replace all (incl. the "old one") of them and update the pull-request.

[chrfin]

@ajcvickers @divega I checked it and Diego is right. Switch is useless in .NET Core as it is only working in-memory per instance.
Now I'm not sure in which direction I should go:

1. Improve the Switch-implementation in CoreFX to be usable
2. Create a new Switch-implementation outside of CoreFX based on Microsoft.Extensions.Configuration (where could/should that be added? new package?)
3. Add Microsoft.Extensions.Configuration to EF6 and use the new config-system instead of the switches
4. Implement some global/static settings directly in EF6 changeable via code

What do you think?
The best solution IMO would be using Microsoft.Extensions.Configuration in the CoreFX-Switch, but that is not possible AFAIK, or is it somehow?

[divega]

@ajcvickers or @bricelam may have good suggestions for this. Just some toughs:

• We cannot add anything that depends on Microsoft.Extensions.Configuration, because the package targets .NET Standard 2.0, and doesn't support .NET Framework 4.0 and 4.5. Also, EF6's design doesn't define a good place to pass something like a configuration instance.

• A change to System.Diagnostics.Switch that would require taking a hard dependency on System.Configuration.ConfigurationManager is unlikely to be accepted because System.Diagnostics.Switch is part of the .NET Core shared framework and this doesn't depend on ConfigurationManager by design. We also don't have a mechanism to do this with a soft dependency.

• Elsewhere we are using AppContext.TryGetSwitch() in particular for quirks you can use to opt out we usually add every time we change a behavior in a patch, but I think unfortunately that API doesn't exist prior to .NET Framework 4.6, so it wouldn't be an option for EF6.

• All that said, EF6 already depends on System.Configuration.ConfigurationManager and we already have a few places in which we read flags from application settings:
  https://github.com/aspnet/EntityFramework6/blob/8d2c24bc00aaa042a1e57eb1070f8ba23448e82a/src/EntityFramework/Core/Objects/ObjectContext.cs#L247
  https://github.com/aspnet/EntityFramework6/blob/22a11d132306c257435fa7d6d75a31359cbbd05a/src/EFTools/EntityDesignerVersioningFacade/ReverseEngineerDb/SchemaDiscovery/EntityStoreSchemaGeneratorDatabaseSchemaLoader.cs#L59

  So I would suggest we go with that.

I am not sure why the code in PlanCompiler uses BooleanSwitch. It is a decision that was made a long time ago when we didn't envision the possibility of running on .NET Core (or we didn't envision .NET Core for that matter 😄), but also it sounds like a setting a user may want to change for non-diagnostic reasons. We could also add an AppSetting option for this one.

[chrfin]

@divega @ajcvickers @bricelam I now changed it to use ConfigurationManager.AppSettings.
I also changed the existing switch - if that is a non-desired breaking-change please tell me and I revert that part...

[chrfin]

@divega May you can take another look? Thanks

[chrfin]

@divega @ajcvickers @bricelam Ping after preview 9 ☺

[divega]

@chrfin sorry for not responding earlier. We looked superficially at the PR and it is looking good, though we may need to do another pass to provide feedback.

I just wanted to answer if this will make it into 6.3: at this time in a release (i.e. with no more previews in sight) we tend not to take changes of this nature as a heuristic to avoid introducing regressions in the last minute. Based on this, I would like us to consider this for the next release.

[chrfin]

Thanks for the feedback and I completely understand your reasoning :-)

[divega]

@chrfin speaking of which, we will sadly have to revert the previous compilation perf improvement because of #1217 😢 We will of course consider it for the next release as well, once we have time to understand what caused it to break things.

[chrfin]

@divega Sad to hear, but also understandable. Could you may keep the lowered MaxNodeCountForTransformations in, as this is already a huge improvement for my app (without that I could not use 6.3 final and would stick to the previews).
I will also take a look at #1217 to may get the changes back in at the first 6.4 preview...

[divega]

cc @bricelam

[ajcvickers]

Thanks for the contribution!