System.NullReferenceException
Joined: 08-Dec-2014
Joined: 08-Dec-2014
Joined: 08-Dec-2014
Joined: 08-Dec-2014
Okay, my hypothesis didn't work: it wasn't because local variable was reused. At the moment, basically I managed to replicate the issue via console app while calling this method with null values in a loop of 10k where each iteration creates 100 tasks and awaits them. Problem happens within 1.5k iterations, usually in hundreds.
I modified the failing LLBLGen method to:
private Delegate AddLambdaExpressionToCache(string key, LambdaExpression expression)
{
if (string.IsNullOrWhiteSpace( key ))
{
File.WriteAllText( @"d:\Temp\llblgen_issue_key_null.txt", key );
}
if (expression == null)
{
File.WriteAllText( @"d:\Temp\llblgen_issue_exp_null.txt", key );
}
AssureDelegateCache();
var toAdd = expression.Compile();
if (_delegateCache == null)
{
File.WriteAllText( @"d:\Temp\llblgen_issue_exp_delegateCache_is_null.txt", key );
}
_delegateCache[key] = toAdd;
return toAdd;
}
Which file do you think was created? 
Answer: llblgen_issue_exp_delegateCache_is_null.txt
I think this is basically a race condition situation:
private void AssureDelegateCache()
{
lock(_delegateCacheLock)
{
if((_delegateCache != null) && (_delegateCache.Count >= CacheMaxEntriesPerThread))
{
_delegateCache = null;
}
if(_delegateCache == null)
{
_delegateCache = new ConcurrentDictionary<string, Delegate>(StringComparer.OrdinalIgnoreCase);
}
}
}
A thread that exits the AssureDelegateCache and then tries to set the value in the dictionary might run into a null-ref if some other thread manages to set that dictionary variable to null in the first if statement.
For now changed the _delegateCache = null to _delegateCache.Clear() to test, over 3.4k iterations passed, will leave the PC and check in several hours.
Btw, it seems like _delegateCache is set to null when cache is full, given I'm invoking the same method all over again I was expecting the cache to contain only single entry, however I recalled that method has DateTime.Now as an additional predicate. Is my understanding of caching correct that basically parameters are also included, i.e. if 500 users will fetch their ids - there will be 500 compilations and entries in the cache?
Enough troubleshooting for now, need to get some sleep - 4:30am
UPDATE: 10k iterations passed
Whoa that's indeed a pretty bad piece of code from our part! 
thanks for testing! You're absolutely right: the _delegateCache=null; shouldn't happen as readers don't use a lock. We'll fix this asap in the runtime.
Every expression that's compiled is cached with a key created from the expression tree. So executing the same query on multiple threads should result in having just 1 entry in the cache, as the expressions lead to the same key. It's the compilation of the lambdas that's slow so we cache them.
We'll check our key generation code as well as it might be we have made a small mistake somewhere so using different thread contexts might lead to different keys for the same expression.
I'll get back to you a.s.a.p.
Joined: 08-Dec-2014
Joined: 08-Dec-2014
Thanks for the into Yes the local variables are embedded in the lambda expression by the compiler (resulting in value(<>c__DisplayClass ... )) and always lead to a different hashcode causing the key to be different every time.
It's basically this: https://petemontgomery.wordpress.com/2008/08/07/caching-the-results-of-linq-queries/
(we use a slightly different approach at places, but the root cause is the same thing). The main problem is that you can't really avoid recompiling the lambda if it has local closures: if you compile the lambda, the first closure is the one used for all other usages as well, which means for the queryfactory all situations where the compiled lambda is re-used will use the first instance of the queryfactory. This isn't OK as it's not a stateless object.
The idea is to reuse the projector lambda for the same query so to avoid recompiling the expression over and over again (as that's not really fast.) however in this case that's not doable. At least I don't see how. We'll think abit about how to avoid all this and still re-use compiled lambdas and not have cache trashing.
We overlooked this completely, and for that my apologies. In our benchmarks we didn't run into this as they don't use local elements in the lambdas so it didn't show up there.
We think the solution is that if the key constructor finds a local object along the way during the visit of the tree, the lambda isn't cached, so it's always compiled. This would still leave expressions at the local level (e.g. Field = SomeEntityFields.SomeField.ToValue<string>() ) get compiled but it won't trash the cache anymore with lambdas it has to recompile anyway.
The projector lambda used to materialize the result is always compiled as there are no local elements left in that lambda.
We hope to have a fixed runtime later today
Joined: 08-Dec-2014
Basically I need to rework all my Select(() => new ...) statements not to have any references to local variables, heh Getting rid of those local properties will make the queries longer and a bit less maintainable, oh well. Checked another option:
return qf.Create()
.Select( productId, productType, companyName )
.From( queryHelper.GetJoinPart( qf ) )
.WithProjector
(
r =>
new ActiveProductListItemPart1
{
Id = (Guid) r[ 0 ],
ProductType = (ProductType) r[ 1 ],
CompanyName = (string) r[ 2 ]
} );
Results in less cache entries (8 vs 12), but a bit uglier, however, can use local variables and, imho, better than Select<ActiveProductListItemPart1> as it's much less mapping error-prone.
Back to cache bloating: then I added one more property:
return await QuerySpecHelper.QueryCollectionViaFactoryWithAdapterAsync( qf =>
{
return qf.Create().Select( () => new ActiveProductListItemPart1
{
// 12 cache entries
Id = ProductFields.Id.Source(productTableAlias).ToValue<Guid>(),
ProductType = ProductFields.ProductType.Source(productTableAlias).ToValue<ProductType>(),
CompanyName = CompanyFields.Name.As(companyNameColumnAlias).Source(companyTableAlias).ToValue<string>(),
MaxPrice = qf.Inventory
.Select(InventoryFields.PricePerItem.Max())
.ToScalar()
.ToValue<int?>(),
Now 12 cached items became 112 (same 100 iterations). Tried replacing the qf.Inventory with new QueryFactory().Inventory - still 112, however, the following version of the query:
return await QuerySpecHelper.QueryCollectionViaFactoryWithAdapterAsync( qf =>
{
var maxPrice = qf.Inventory.Select(InventoryFields.PricePerItem.Max()).ToScalar();
return qf.Create()
.Select(productId, productType, companyName, maxPrice)
.From(queryHelper.GetJoinPart(qf))
.GroupBy(productId, productType, companyName)
.WithProjector
(
r =>
new ActiveProductListItemPart1
{
Id = (Guid)r[0],
ProductType = (ProductType)r[1],
CompanyName = (string)r[2],
MaxPrice = (int?)r[3]
});
results in 8. It was also 8 before I added the MaxPrice property.
Joined: 08-Dec-2014
Otis wrote:
We think the solution is that if the key constructor finds a local object along the way during the visit of the tree, the lambda isn't cached, so it's always compiled. This would still leave expressions at the local level (e.g. Field = SomeEntityFields.SomeField.ToValue<string>() ) get compiled but it won't trash the cache anymore with lambdas it has to recompile anyway.
The projector lambda used to materialize the result is always compiled as there are no local elements left in that lambda.
We hope to have a fixed runtime later today
So with this change I can keep on using local variables, but they won't pollute the cache, however, will be compiled anyway? How much performance penalty do you think that is? I'm somehow now leaning towards ditching expression based .Select() and move to .WithProjector()... hm
Btw, would it make sense to have some diagnostics (trace?) in the run-time that could help debugging issues like that. Cache size would have been nice, if value would be growing and then resetted we'd know cache is being polluted. Also for lambda compilations: allowing cheaper ones to stay as it, but more expensive ones to be reworked to .WithProjector(). To my understanding the .WithProjector() approach is the best so far
Findev wrote:
Otis wrote:
We think the solution is that if the key constructor finds a local object along the way during the visit of the tree, the lambda isn't cached, so it's always compiled. This would still leave expressions at the local level (e.g. Field = SomeEntityFields.SomeField.ToValue<string>() ) get compiled but it won't trash the cache anymore with lambdas it has to recompile anyway.
The projector lambda used to materialize the result is always compiled as there are no local elements left in that lambda.
We hope to have a fixed runtime later today
So with this change I can keep on using local variables, but they won't pollute the cache, however, will be compiled anyway? How much performance penalty do you think that is? I'm somehow now leaning towards ditching expression based .Select() and move to .WithProjector()... hm
Not that much, the main performance bottleneck with lambdas is when the data is materialized and that lambda is always compiled. (as all local variables enclosed in the lambda have been evaluated and replaced). Compiling the lambda is of course a bit slower than having no compilation but it's not the end of the world. I don't think you'll see any performance degradation as the time the query takes to get executed and the data gets materialized is larger than the time it takes to evaluate the projection.
You'll also only have a lambda get compiled every time when you use the e.g. query factory inside the projection (as it's an instance that's enclosed). A straight forward projection with only ToValue<>() calls doesn't use a local variable and thus will be compiled once. Your approach with local variables is going to get recompiled every time but you already are doing that behind the scenes (as the cached lambda is never reused) so it'll give the same performance as you have now.
Btw, would it make sense to have some diagnostics (trace?) in the run-time that could help debugging issues like that. Cache size would have been nice, if value would be growing and then resetted we'd know cache is being polluted. Also for lambda compilations: allowing cheaper ones to stay as it, but more expensive ones to be reworked to .WithProjector(). To my understanding the .WithProjector() approach is the best so far
that's a good idea, the cache pollution one. When the cache is cleared because it's full it's good to notify that so people get an idea what's going on (like query plan recompilation on sql server).
Regarding WithProjector, what the Select( ()=>new ... ) does is effectively reworking it to a WithProjector like projection, so you can use the WithProjector approach to avoid any closures, but it's a bit less readable.
We'll have a fix read in an hour
Joined: 08-Dec-2014
Let me try to sum it up.
1) With the current run-time and I have and the one with the upcoming fix: .WithProjector() is the safest and sort of the best(?) for caching: no closures, less cache entries=less compilations which is good, but less readable.
2) .Select(expression): with current run-time and I have and the one with the upcoming fix: I have to get rid of ANY local variables inside the projection expression. Once this is done, cache will be properly reused as keys will be matching. Minor downside: code will be less readable because .As(), .Source() etc will have to be spread out to corresponding properties.
3) .Select(expression) + using query factory for subqueries: with current run-time and I have and the one with the upcoming fix: will always lead to generation of new cache entry, thus queries like that should be reworked to use .WithProjector()
4) I will still be able to see how cache is growing by having the Console.WriteLine in the modified sources as I have now to assist my refactoring
Downside: have to tinker with public + token and skip sn validation, but that's already automated
5) Later there will be a better way to get that information from the run-time. Maybe even query directly (static variable?), that might be more helpful and I can then try to check that value, say, while looping the test(s) and catch if cache is growing which would mean that some queries needs to be fixed (local variables) so that cache key is matching.
Replying per point is very much appreciated
P.S.: please don't forget to put up the sources for the fixed run-time
Hotfix is now available (also on nuget)
Findev wrote:
Let me try to sum it up.
1) With the current run-time and I have and the one with the upcoming fix: .WithProjector() is the safest and sort of the best(?) for caching: no closures, less cache entries, but less readable.
It's indeed the safest wrt to recompiles, as there is just 1 compile: of the lambda you provide, and only once. As long as there's no variable enclosed in the lambda (and normally you don't have any variables enclosed with this approach) you won't have this lambda getting recompiled.
2) .Select(expression): with current run-time and I have and the one with the upcoming fix: I have to get rid of ANY local variables inside the projection expression. Once this is done, cache will be properly reused as keys will be matching. Minor downside: code will be less readable because .As(), .Source() etc will have to be spread out to corresponding properties.
Correct. But don't worry too much about recompiles, as the key generator now properly detects if the expression can be cached and if not, it will recompile it and not cache it, so you won't see cache trashing anymore (at least that's what it should do, we couldn't find any cache trashing anymore).
Any static variables are not local enclosures btw. If you have a static field / property / method, it's seen as something that always returns the same thing so the object returned will always result in the same hashcode for the key so using static variables won't lead to cache trashing nor recompiles. Of course the elements have to be reusable. If you wrap your variables in static methods the compiled lambda will simply call the static method when it's called so the compiled form of the lambda is cachable and your cache shouldn't get full as the key is always the same.
3) .Select(expression) + using query factory for subqueries: with current run-time and I have and the one with the upcoming fix: will always lead to generation of new cache entry, thus queries like that should be reworked to use .WithProjector()
Not needed. The new runtime will detect this and avoid caching the lambda you provide, so it will recompile it again every time the query is created. This is done with the old runtime as well. The lambda used for projection and created from this expression in the end is compiled once. So you don't need to rework these: you won't lose performance over what you have now and it won't trash the cache.
4) I will still be able to see how cache is growing by having the Console.WriteLine in the modified sources as I have now to assist my refactoring Downside: have to tinker with public + token and skip sn validation, but that's already automated
If you want to pull the cache, and observe the count, use:
var cache = typeof(ProjectionLambdaTransformer).GetField("_delegateCache", BindingFlags.Static | BindingFlags.NonPublic);
var count = ((IDictionary)cache.GetValue(null)).Count;
So you don't need to modify the sources
5) Later there will be a better way to get that information from the run-time. Maybe even query directly (static variable?), that might be more helpful and I can then try to check that value, say, while looping the test(s) and catch if cache is growing which would mean that some queries needs to be fixed (local variables) so that cache key is matching.
It should now be the case that no query should pollute the cache anymore with a lot of expressions which are never re-used as several checks are now in place to verify if the expression contains elements which are local / enclosed: if so, the key is void and the expression isn't cached.
We'll later on add a trace output.
P.S.: please don't forget to put up the sources for the fixed run-time
Done.
Please let us know if this still causes problems
Joined: 08-Dec-2014
Take away: don't change anything, but test with new run-time. If I want the safest and best performance go with .WithProjector(). Evaluate if a bit worse readability costs are justified to gained benefits.
Alright, will make tea, pull new sources, nugets and start testing then. Thank you!
Joined: 08-Dec-2014
Findev wrote:
Modified new sources the same way:
AssureDelegateCache(); var toAdd = expression.Compile(); Console.WriteLine( $"Cache size: {_delegateCache.Count}..." ); _delegateCache[key] = toAdd; return toAdd;When following is executed:
var tasks = Enumerable.Range(0, 1).Select (x => Task.Run ( () => { var id = Guid.NewGuid(); return userService.GetUserLightAsync(id); }));I got 8 messages about the cache size (0, 1, 2, 3, 4, 5, 6, 7)
when Enumerable.Range(0, 1) is changed to Enumerable.Range(0, 100) then I'm seeing duplicates in the output and total number of lines range from ~160-~<210. Is that an expected behavior?
Hmm. the GetUserLightAsync method, what does it do exactly? The duplicate lines might be because it runs on multiple threads? (so the chance is higher with a large range that 2 or more threads reach the add-to-cache line at roughly the same time...
That the cache increases with that code suggests there's still a situation where the key generator doesn't spot a local element (which can appear in various forms, hence it's not entirely simple to detect it... )
UPDATE: of course this is nowhere near proper benchmarking, but 300 iterations with 100 tasks each executed 3 times each one after another: WithProject(): 12:11, 14:54, 11:51 Select + expression: 11:27, 11:36, 11:30 seems like lambda is a bit faster...
Interesting the difference might be coming from the boxing that's occurring in the withprojector lambda (as it uses an object array) which is slower on .net core than on .netfx (compared to typed fetches). other than that I can't explain the difference.
Joined: 08-Dec-2014
Yes, lots duplicate lines appear when 100 tasks per iteration are executed, then it stops and iterations continue without caching anymore. That's a service method which basically proxies to the repository:
public async Task<UserLight> GetUserLightAsync(string email)
{
if (string.IsNullOrWhiteSpace(email))
{
throw new ArgumentNullException(nameof(email));
}
var result = await GetUsersLightInternalAsync( UserFields.Email == email ).ConfigureAwait( false );
return result.SingleOrDefault();
}
private async Task<IList<UserLight>> GetUsersLightInternalAsync( IPredicate filter, ISortClause[] sortClauses = null, PagingParameters? paging = null )
{
if (filter == null)
{
throw new ArgumentNullException( nameof(filter) );
}
// ReSharper disable once AsyncConverter.AsyncAwaitMayBeElidedHighlighting
return await QuerySpecHelper.QueryCollectionViaFactoryWithAdapterAsync(
qf =>
{
var q = GetUserLightQueryBody( filter, qf );
if (sortClauses?.Any() == true)
{
q = q.OrderBy( sortClauses );
}
if (paging != null)
{
q = q.Page( paging.Value.PageNumber, paging.Value.PageSize );
}
return q;
} )
.ConfigureAwait( false );
}
private static DynamicQuery<UserLight> GetUserLightQueryBody( IPredicate filter, QueryFactory qf )
{
return qf.Create<T>()
.Where( filter )
.Select(
() =>
new UserLight
{
Id = UserFields.Id.ToValue<Guid>(),
Email = UserFields.Email.ToValue<string>(),
FirstName = UserFields.FirstName.ToValue<string>(),
LastName = UserFields.LastName.ToValue<string>(),
Culture = UserFields.Culture.ToValue<string>(),
TimeZoneId = UserFields.TimeZoneId.ToValue<string>(),
DateCreated = UserFields.DateCreated.ToValue<DateTime>()
} );
}
As for yesterday's "benchmark" I will test with local variables, noticed that lambda was:
Id = ProductFields.Id.Source(productTableAlias).ToValue<Guid>(),
ProductType = ProductFields.ProductType.Source(productTableAlias).ToValue<ProductType>(),
CompanyName = CompanyFields.Name.As(companyNameColumnAlias).Source(companyTableAlias).ToValue<string>(),
MaxPrice = qf.Inventory
.Select(InventoryFields.PricePerItem.Max())
.ToScalar()
.ToValue<int?>(),
will see if there's difference when it is:
Id = productId.ToValue<Guid>(),
ProductType = productType.ToValue<ProductType>(),
CompanyName = companyName.ToValue<string>(),
MaxPrice = qf.Inventory
.Select(InventoryFields.PricePerItem.Max())
.ToScalar()
.ToValue<int?>(),
Thank you!
UPDATE: my timing "benchmarks" can be ignored, they are not consistent, so I just take your word about performance and keep on using expressions in my selects