Make ConcurrencyDetector optional

[roji]

ConcurrencyDetector uses an AsyncLocal to allow reentrant async usages; AsyncLocal triggers lots of ExecutionAsync allocations, plus the bool that we set gets boxed internally:

A quick search in the codebase shows that we don't actually need the reentrancy.

OK yeah, Lazy_load_one_to_one_reference_with_recursive_property requires reentrancy, as well as queries being executed in the top-most projection (e.g. Select_nested_projection, Top_level_projection_track_entities_before_passing_to_client_method), which I think also require MARS.

[roji]

FYI removing this AsyncLocal is worth almost 5% RPS in TE fortunes perf (from 74,909 RPS to 77,149).

[roji]

Design decision: add a configuration flag to remove concurrency detection entirely (not just reentrancy).

[roji]

This adds the concurrency detector flag as a core singleton option, similar to detailed errors. An alternative would be to just register a different no-op implementation of IConcurrencyDetector, but it seems a bit better to bake the flag into the querying enumerable compiled query (we could in theory even remove the check entirely).

[smitpatel]

Avoid usage of collection and assign different store name in derived types. Then your cosmos test issue would also be resolved.

[roji]

Don't we want to minimize the number of databases we create though?

Re the cosmos issue, I think the fix in this PR should go in anyway, since it makes the cosmos testing infrastructure behave more like the other providers'.

Basically I don't really care so much :)

[smitpatel]

I am fine either way, so far we have gone with different names rather than using collection (perhaps because model always differed). It is trade-off of new pattern vs additional cost. May be @AndriySvyryd can suggest, or go with whatever you feel like.

[AndriySvyryd]

With the Cosmos fix you should be able to use the same store name and avoid using collection, right?

[roji]

Note that we have SaveChanges which actually changes the database; without the collection, wouldn't the two classes run in parallel and therefore interfere with one another?

I can have the SaveChanges tests from the different classes look for different rows, so that even if they run in parallel they don't interfere, but it may be safer for the future to just prevent parallelization for this specific test suite... Let me know what you prefer and I'll do it.

[smitpatel]

If we can look for different rows and avoid this then it would be better. Imagine some random intern in future removing collection from here trying to improve test perf. It passes locally but failed the official build. 😉

[roji]

Random future intern fiddling around with our tests makes me scared! Will change.

[ghost]

Hello @roji!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.