-
Notifications
You must be signed in to change notification settings - Fork 243
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
Rethinking Include implementation #187
Comments
Also to comply with efcore's extension methods, |
Hi @devbased, Thank you, we appreciate your effort here. I'll analyze your fork in more detail soon. At first glance, I have a few remarks. Let's have the following specification public class StoreSpec : Specification<Store>
{
public StoreSpec(StoreFilter filter)
{
Query.Include(x => x.Products.Where(x => x.Name.Equals(filter.Name) && x.Descriptions.Contains(filter.DescriptionSearch))
.ThenInclude(x => x.CustomFields.Where(x => x.Id == filter.CustomFieldId);
}
}
Also, I don't think you need to replace Thanks! |
@fiseni Thank you for the feedback! Now i see downsides and that this problem is not quite trivial |
No need for disappointment, the road almost always is bumpy 😄 We'll think about it, but I don't think caching would be a smart idea (especially I try to avoid it in libraries). In this case, it's even a security concern. A malicious user can provide random strings as parameters, and your cache will grow... until eventually, something crashes. |
Cache grow could be resolved by expiration or using something like LRU. |
Indeed you can. But, what will be your limit? How are you gonna decide for that for a broad audience? If the limits are small, the performance gains will be somewhat trivial, and if you set them high you're messing with the user's intentions. I mean we're stepping in a different realm there, with a whole set of new problems. Even EF is not caching the query expressions. In EF Core 6, they're compiling the model and some of the queries I think, but it's a tricky subject. In any case, we appreciate your work here. We'll think about how we can evolve this 👍 |
Wait, i don't cache user's expressions. I cache 'proxy' delegates which used to pass user's expressions to efcore extension methods 🤔 |
Yeah, my point was that not even EF was going toward that idea. You're caching the delegates, but you have an issue with the cache keys. It will grow, so still, you will need some expiration there. Also, please try if your changes actually work. Run the integration tests or do some additional ones. It seems EF extension methods are not called correctly, and internally EF bypasses the logic and just returns the source. That's why you're faster than EF in the tests. |
Hey @devbased, Now I'm checking your fork and your changes. I was wrong about the cache keys, that's not an issue. Let's have the following specifications public class StoreSpec1 : Specification<Store>
{
public StoreSpec1()
{
Query.Include(x => x.Products);
}
}
public class StoreSpec2 : Specification<Store>
{
public StoreSpec2(StoreFilter filter)
{
Query.Include(x => x.Products.Where(x => x.Name.Equals(filter.Name));
}
} The Include expressions in these specifications, even though they are different, will map to the same cached delegate. But, that's not an issue actually. All you care about are the generic parameters, and in both cases they're same. It looks promising. Let's test it a bit more and think of possible issues. Now that I'm checking your code, it doesn't seem we'll have an issue with closures here. |
Actually, caching could be opt-in by some option in IncludeEvaluator. Without caching just do this query = (IQueryable<T>)EfCoreIncludeMethodInfo.MakeGenericMethod(includeInfo.EntityType, includeInfo.PropertyType).Invoke(null, new object[] { query, includeInfo.LambdaExpression, }); There will be reflection overhead but still we'll use efcore public API. |
Yes, that's reasonable. Enabling it per specification might not be nice. Perhaps we can provide that possibility in the specification evaluator. public class SpecificationEvaluator : ISpecificationEvaluator
{
// Will use singleton for default configuration. Yet, it can be instantiated if necessary, with default or provided evaluators.
public static SpecificationEvaluator Default { get; } = new SpecificationEvaluator();
public static SpecificationEvaluator Cached { get; } = new SpecificationEvaluator(true);
private readonly List<IEvaluator> evaluators = new List<IEvaluator>();
public SpecificationEvaluator(bool cacheInclude = false)
{
this.evaluators.AddRange(new IEvaluator[]
{
WhereEvaluator.Instance,
SearchEvaluator.Instance,
cacheInclude ? (IEvaluator)IncludeEvaluatorCached.Instance : (IEvaluator)IncludeEvaluator.Instance,
OrderEvaluator.Instance,
PaginationEvaluator.Instance,
AsNoTrackingEvaluator.Instance,
#if NETSTANDARD2_1
AsSplitQueryEvaluator.Instance,
AsNoTrackingWithIdentityResolutionEvaluator.Instance
#endif
});
}
public SpecificationEvaluator(IEnumerable<IEvaluator> evaluators)
{
this.evaluators.AddRange(evaluators);
}
...
} Then users can opt-in for the cached version as following public class MyRepository<T> : RepositoryBase<T>, IRepository<T> where T : class
{
private readonly SampleDbContext dbContext;
public MyRepository(SampleDbContext dbContext) : base(dbContext, SpecificationEvaluator.Cached)
{
this.dbContext = dbContext;
}
// Not required to implement anything. Add additional functionalities if required.
} |
Thanks @devbased, it looks great. Would you like to create a PR? Btw, we have to do the same for EF6 package. Would you like to work on it too? If yes, please let's include it in the same PR. |
@fiseni I'm not familiar with ef6 (for me it is deprecated😅), so i need to find a time to take a look what could be done. |
@fiseni EF6 supports Include method with string argument and provides extension to pass lambda expression which is parsed to string anyway. IQueryable<Store>.Include(x => x.Company.Stores.Select(x => x.Products))
// converted internally to
IQueryable<Store>.Include("Company.Stores.Products") I see a couple of options:
|
Indeed, there is no Anyhow, we also provide an overload for So, yeah, we'll leave it as it is, and won't implement caching for EF6. |
As said @fiseni in #182 (comment)
It would be great to change 'Include' implementation in terms of performance and logic encapsulation (currently we're using EFCore internals which is not reliable).
Proposal
Dynamically construct and call following delegates in
IncludeExtensions.Include
orIncludeExtensions.ThenInclude
IIncludableQueryable<TEntity, TProperty>EntityFrameworkQueryableExtensions.Include<TEntity, TProperty>(IQueryable<TEntity>, Expression<Func<TEntity, TProperty>>)
IIncludableQueryable<TEntity, TProperty> EntityFrameworkQueryableExtensions.ThenInclude<TEntity, TPreviousProperty, TProperty>(IIncludableQueryable<TEntity, TPreviousProperty>, Expression<Func<TPreviousProperty, TProperty>>)
IIncludableQueryable<TEntity, TProperty> EntityFrameworkQueryableExtensions.ThenInclude<TEntity, TPreviousProperty, TProperty>(IIncludableQueryable<TEntity, IEnumerable<TPreviousProperty>>, Expression<Func<TPreviousProperty, TProperty>>)
Instead of copying efcore logic
Do this
Note: CachedReadConcurrentDictionary is thread-safe dictionary for read-heavy workloads
Pros and cons
Pros
Expression.Call
)Cons
Implications
Should be replaced with
Or even consolidated with this one since
typeof(TPreviousProperty)
==typeof(IEnumerable<TPreviousProperty>)
(branching will be done in CreateThenIncludeDelegate)Include
method return type should be changed fromIQueryable<T>
toIIncludableQueryable<T>
. AndThenInclude
source parameter type fromIQueryable<T>
toIIncludableQueryable<T>
. Or move this logic directly to the evaluator.Benchmark
Modified #83 (comment)
The text was updated successfully, but these errors were encountered: