NhLinqExpression is instantiated in DefaultQueryProvider.PrepareQuery():
protected NhLinqExpression PrepareQuery(Expression expression, out
IQuery query, out NhLinqExpression nhQuery)
{
var nhLinqExpression = new NhLinqExpression(expression,
Session.Factory);
query = Session.CreateQuery(nhLinqExpression);
...
As I understand the code, it is the call to CreateQuery() that will lead to
a query plan cache (QPC) lookup, and if no cached plan is found it will
eventually call NhLinqExpression.Translate() before CreateQuery() returns.
The proposed change, moves work from Translate() to the NhLinqExpression
ctor. Unfortunately this effectively nullifies a large part of the purpose
with the QPC, by having NH do the entire linq-to-hql-ast translation even
before looking in the QPC.
If I'm right about the above, a simple change might be to just have the
Translate() method nullify the _expression variable after it's done with it
(and have it skip the translation code if ExpressionToHqlTranslationResults
is already non-null).
Comments on this?
(On a somewhat tangential point, I feel the entire interaction between the
linq implementation, QPC and the session implementation is rather messy
code...)
/Oskar
2013/12/16 Lauri Kotilainen <[email protected]>
> Yeah... an actual case of "select is broken". Who would have thought.
>
> Jira issue:
> https://nhibernate.jira.com/browse/NH-3579
>
> Pull request:
> https://github.com/nhibernate/nhibernate-core/pull/244
>
> I based the patch off the master branch -- figured that someone else will
> be a better judge of what to backport to earlier versions.
>
> -Lauri
>
>
> On Monday, December 16, 2013 1:15:21 PM UTC+2, Gunnar Liljas wrote:
>
>> It's troubling that the leak occurred in the first place.
>>
>> /G
>>
>>
>> 2013/12/15 Lauri Kotilainen <[email protected]>
>>
>> Which part is troubling? The part about the leak or the part about
>>> having the query plan cache end up constructing the expression?
>>>
>>> If it's the latter, I did try other approaches too. I don't really like
>>> heavy ctors either, but the QPC and NhLinqExpressions are used in a variety
>>> of ways, and all the alternatives I could think of ended up causing
>>> breakage all over the place. This might be due to the fact that I don't
>>> have a very good mental map of the dependencies yet, though.
>>>
>>> I'll file an issue soon-ish with the changes you proposed to the test.
>>>
>>> -Lauri
>>>
>>> On Friday, December 13, 2013 2:41:36 PM UTC+2, Gunnar Liljas wrote:
>>>
>>>> So far it looks OK, although it's a bit troubling that the query plan
>>>> cache does this. I'm not a big fan of heavy constructors, but in this case
>>>> it makes some sense.
>>>>
>>>> I think you can make your test behave correctly (i.e fail when it
>>>> should) by adding a second pass of GC.Collect()
>>>>
>>>> A JIRA issue and an accompanying unit test (for now, remove the
>>>> dependency on SQLite) would be nice.
>>>>
>>>> /G
>>>>
>>>>
>>>> 2013/12/12 Gunnar Liljas <[email protected]>
>>>>
>>>>> Great work, Lauri!
>>>>>
>>>>> I'll do some tests tomorrow, just to give you feedback.
>>>>>
>>>>> /G
>>>>>
>>>>>
>>>>>
>>>>> 2013/12/11 Lauri Kotilainen <[email protected]>
>>>>>
>>>>> Hi,
>>>>>>
>>>>>> I originally posted the description of this issue to the nhusers list:
>>>>>>
>>>>>> https://groups.google.com/d/topic/nhusers/v_6WCod79XE/discussion
>>>>>>
>>>>>> and I won't waste bits by pasting the entire description here unless
>>>>>> it's deemed necessary. Anyway, I think I have a patch that fixes the
>>>>>> session leak, but I don't understand the big picture well enough to
>>>>>> evaluate whether or not it's a safe change. Essentially, what I did was
>>>>>> move most of the code from NhLinqExpression.Translate to the
>>>>>> NhLinqExpression constructor and eliminated the _expression field, making
>>>>>> it a local variable in the ctor. It didn't cause any test failures in the
>>>>>> master branch, and after I backported it to the 3.4.x branch and tested
>>>>>> with the problematic application, the leak is gone (according to ANTS
>>>>>> profiler).
>>>>>>
>>>>>> Here's the patch:
>>>>>> https://gist.github.com/rytmis/3735cfc274e135aae753
>>>>>>
>>>>>> Unfortunately, even with that patch applied, the unit test in my
>>>>>> other post fails -- even though a heap inspection with WinDBG confirms
>>>>>> that
>>>>>> the session is no longer rooted and is eligible for collection.
>>>>>>
>>>>>> What I would like to know at this point is whether the change is
>>>>>> likely to cause performance regressions or other unexpected side effects.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -Lauri
>>>>>>
>>>>>> --
>>>>>>
>>>>>> ---
>>>>>> You received this message because you are subscribed to the Google
>>>>>> Groups "nhibernate-development" group.
>>>>>> To unsubscribe from this group and stop receiving emails from it,
>>>>>> send an email to [email protected].
>>>>>> For more options, visit https://groups.google.com/groups/opt_out.
>>>>>>
>>>>>
>>>>>
>>>> --
>>>
>>> ---
>>> You received this message because you are subscribed to the Google
>>> Groups "nhibernate-development" group.
>>> To unsubscribe from this group and stop receiving emails from it, send
>>> an email to [email protected].
>>> For more options, visit https://groups.google.com/groups/opt_out.
>>>
>>
>> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "nhibernate-development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to [email protected].
> For more options, visit https://groups.google.com/groups/opt_out.
>
--
---
You received this message because you are subscribed to the Google Groups
"nhibernate-development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
