-
Notifications
You must be signed in to change notification settings - Fork 244
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
Improving specification evaluation performance #182
Comments
Hey @devbased, First of all, thanks for your interest in doing this. This is only related to in-memory evaluators (in this case you demonstrated for the search evaluator).
Anyhow, this was great work, thank you. I'll analyze it further next week, and see how we can benefit from it in the best way. If we don't see any downside of doing this, then why not, we'll implement it. To be honest, I don't care so much about the in-memory evaluators 😄 We have a bigger issue with Include chains, in terms of performance. We are forced to keep the state in form of LambdaExpressions, and then construct the IQueryable through reflection (it's done because of multiple inner return types). I'll be quite happy if anyone has better ideas on that subject. That would be really great improvement. |
Good point. But i think if library will be well structured and optimized for most use cases it will encourage people to stick to this approach. Like you have said, user basically has a bunch of specifications and he's free to use them with any collection, doesn't matter in-memory or persisted one. They could be cached across requests it's up to user to decide to keep spec instance somewhere or not. But for now even if they keep it, it won't give much. About include lambdas, not sure if i understand where's heavy part. |
You're right. Perhaps we have to give that flexibility to the users. Anyway, this will affect only the in-memory evaluations. Regarding the For context, read this PR #83 and this issue |
Hey @devbased I just took a look at your proposal. Let's implement it, but with the following considerations:
Proposed implementation: public interface ISpecification<T>
{
IEnumerable<SearchExpressionInfo<T>> SearchCriterias { get; }
}
public class SearchExpressionInfo<T>
{
private readonly Lazy<Func<T, string>> selectorFunc;
public SearchExpressionInfo(Expression<Func<T, string>> selector, string searchTerm, int searchGroup = 1)
{
_ = selector ?? throw new ArgumentNullException(nameof(selector));
if (string.IsNullOrEmpty(searchTerm)) throw new ArgumentNullException(nameof(searchTerm));
this.Selector = selector;
this.SearchTerm = searchTerm;
this.SearchGroup = searchGroup;
this.selectorFunc = new Lazy<Func<T, string>>(this.Selector.Compile);
}
public Expression<Func<T, string>> Selector { get; }
public string SearchTerm { get; }
public int SearchGroup { get; }
public Func<T, string> SelectorFunc => this.selectorFunc.Value;
} |
@fiseni Seems good. Should i remove benchmark project? |
Yes, please remove the benchmark project. We might need something more generic. |
Note: This is a breaking change for all users that have been accessing the specification state directly. We should document it in release notes. |
For those of use that have been accessing specification state, what is the fix for this breaking change? This stopped working and is throwing an invalid cast exception on 6.0.0 public enum SortOrder
{
ASC,
DESC
}
public static IOrderedSpecificationBuilder<T> OrderBy<T>(
this ISpecificationBuilder<T> specificationBuilder,
Expression<Func<T, object>> orderExpression,
SortOrder orderBy)
{
((List<(Expression<Func<T, object>> OrderExpression, OrderTypeEnum OrderType)>)specificationBuilder
.Specification.OrderExpressions)
.Add(orderBy == SortOrder.DESC
? (orderExpression, OrderTypeEnum.OrderByDescending)
: (orderExpression, OrderTypeEnum.OrderBy));
var orderedSpecificationBuilder = new OrderedSpecificationBuilder<T>(specificationBuilder.Specification);
return orderedSpecificationBuilder;
}
|
You can find the answer in this issue. |
I don't like current spec evaluation because it compiles expressions with every call to
Evaluate
.So i have added benchmark project to provide ability to measure performance and replaced
IEnumerable<(Expression<Func<T, string>>, string, int)> ISpecification{T}.SearchCriterias
withIEnumerable<SearchExpressionBase<T>>
. There also 2 approaches to compile expressions: one is default dotnet compile and second is CompileFast from dadhi/FastExpressionCompiler.With this changes user code could cache specifications to not just reduce amount of allocations but also to avoid recompilation.
I'm not sure if it's super precise benchmark since i don't have enough experience in such things and i've ran it on my home PC, however everyone can do it by himself to verify results.
I need to know, should i continue and do the same for other expressions or not?
Long story short, here's the code and benchmark results
Before
After
The text was updated successfully, but these errors were encountered: