Double query translation

[fredericDelaporte]

This is a kind of follow-up to #1544, for minimizing its potential impact on performances. But it also improves things for other cases.

(By the way, I have checked with the "raw data benchmark", #1544 was not having a measurable impact on it, even by tweaking it for using a Linq query with a non-parameterized constant. But the way it works, pulling a whole table without paging, the incurred IO are quite big and so it is not really surprising we cannot see the impact of a potential performance loss occurring only once per query.)

First commit rationals:

(Avoid re-translating NhLinqExpression)

Whatever the querying API, executing a query currently involves two steps:

• Create the query (prepare in LINQ)
• Set parameters (not exposed to users with LINQ)
• Execute the query

First and last steps need the query plan, and these steps ask it from the plan cache, which will generate it if it is missing.
The first step needs it notably to initialize its parameter list. The last step of course needs it for executing the query.

So if a plan is not cacheable (which since #1544 may happen with LINQ queries), it will forcibly be regenerated at the execution step, instead of being retrieved from cache.

At the first step, the generated plan cannot be stored in the query for later re-use, because the query or its execution conditions may be altered and may require another plan for the query execution. This occurs:

• When a query use a list parameter. Such a parameter has to be replaced by as many parameters as it has values in its list. (Moreover in the LINQ case this query alteration needs the original query translation, and recomputes it.)
• For non Linq queries, the user may enable session filters after having created a query. Query plan keys depend on the list of enabled filter.

But the query translation, done when generating a plan, can be re-used if the query expression instance is not changed for another one. So this PR alter NhLinqExpression, which was already storing its translation results, for yielding them again instead of recomputing them in case of later Translate call.

This change is not done for non-LINQ queries because they are string based, without a common object persisting from first plan look-up to second look-up. Anyway, they would less benefit of the change since non-LINQ query plans are currently always cacheable.

By the way this change lighten an old "re-translation" concern occurring with queries having list parameters, see code comments changes.

Second commit:

(Fix a wrong parameter value for query translation)

The first commit, as you can read in its code comment, assumes a query is always translated for the same session factory and with the same bool filter parameter. This was not true for filter due to a latent bug causing this parameter to have a wrong value for some filter query case. But this bug was not having any impact because this parameter is currently unused.

The second commit just fix that, for having a cleaner code. It currently changes nothing to the feature behavior, so we could also just drop that commit.

[hazzik]

Typos.

Any tests required?

[hazzik]

matter (no "s")

[hazzik]

"on a mapped collection"

[fredericDelaporte]

Typos fixed, thanks.

Any tests required?

Besides running existing ones and seeing they still succeed, no additional ones.
With query plan there are logs allowing to checks how many generations or cache hit occurred. But there are none in the case of query translation, and I rather prefer not adding logs just for tests. There are also too many things involved in triggering the query translation for elaborating a NSubstitute based test.

[fredericDelaporte]

Cases which are re-using the translation were failing because the query plan compilation alters the supplied query translation (AstNode tree), although it also creates a new nodes tree. This happens in HqlSqlTranslator.Translate(). In it, it is obvious that the filter case always mutates the input. But moreover, it appears that HqlSqlWalker.statement() also mutate the input, although this is not obvious (and maybe not expected indeed). When looking at AstNode.ToStringTree(), we can see that the entity alias (e by example) gets replaced by the sql alias (entity0_) excepted in the from clause.
Fixing the filter case for generating a new tree instead of mutating the input would be easy, there are already similar cases for parameter expansion and polymorphic tree building.
But about the HqlSqlWalker, I get lost trying to find where it does mutate its input.

Since the AstNode returned by NhLinqExpression.Translate was previously never re-accessed from NhLinqExpression instances (neither inside them nor from using their properties), having it mutated was not having any impact. (Neither needed nor causing bug.) But it prevents re-using it inside this PR.

The more robust solution is then to yield a copy of the AstNode instead of the stored one. This may be a bit redundant in the polymorphic case, where the tree is always duplicated. So we could instead "hack" the polymorphic detector for yielding a duplicated tree in non-polymorphic cases too. But that is not its responsibility, so I have ruled out this option.

The other option would be to avoid mutating the input when building the query plan. But this looks like requiring many more code changes, with some cases which will anyway have to duplicate the whole AstNode tree (filter case).

[fredericDelaporte]

Although I think the "raw data access bench" is not very suitable for checking impact of this change, I have still run it: no significant differences in timings.

[hazzik]

no significant differences in timings.

Still, better or worse?

[fredericDelaporte]

Below standard deviation, so cannot say. I will put the figures later. But It think a different bench is needed for really checking. Like one that runs queries having lightweight results in parallel and compute the average throughput.
On Linq queries not needing to recompute their plan, it may show a loss, while on queries recomputing their plan (not cacheable ones due to non-parameterized constant + those having a list parameter with a cardinality other than one) it may show a win. But as those may be smaller than the variability of the database response time, it is not even sure.
For better checking it should instead be a test using a driver mock yielding in-memory results. (In such case better no more run it parallel for minimizing impact of other OS's processes which may cause random competing load.)

[fredericDelaporte]

Test	NH prior to the PR	NH altered by this PR	% (base is prior to the PR)
Change tracking fetches, set fetches (50 runs), no caching	1 369.71ms, Enum: 1.71ms, 206 155KB	1 352.97ms, Enum: 1,61ms, 206 147KB	-1.3%ms, Enum: -5.8%, -0.0004%KB
Change tracking individual fetches (100 elements, 50 runs), no caching	0.32ms, 64KB	0.37ms, 64KB	+15.6%ms, 0%KB
Change tracking fetches, eager load fetches, 3-node split graph, 1000 root elements (50 runs), no caching	179.38ms, 30 768KB	178.94ms, 30 768KB	-2.5%ms, 0%KB

So that seems like this PR would be better for tests involving LINQ (first and third).
But, as the individual fetches (second line) does not involve any LINQ (it is implemented with an ISession.Get), it demonstrates the lack of reliability of comparing those figures when they are close. (Standard deviation is 2ms for this test, so even differences beyond standard deviation are not reliable.)

[fredericDelaporte]

I have added a test fixture (set as explicit) adequate for benchmarking the change.

Raw results:

With 1547:
Run 1
List parameter average time: 22,5247193ms (s 4,333056688069ms)
Non parameterized constant average time: 23,4214071ms (s 3,9677044980547ms)
Simple LINQ average time: 8,0586627ms (s 2,548815048447ms)

Run 2
List parameter average time: 22,426151ms (s 4,65178411170821ms)
Non parameterized constant average time: 23,1536621ms (s 3,53700987800864ms)
Simple LINQ average time: 7,8267019ms (s 2,90186192777228ms)

Without 1547:
Run 1
List parameter average time: 25,4970112ms (s 5,54136760824837ms)
Non parameterized constant average time: 35,9412004ms (s 4,5727023529384ms)
Simple LINQ average time: 7,7744208ms (s 2,72784471485745ms)

Run 2
List parameter average time: 25,2826661ms (s 5,19295881027821ms)
Non parameterized constant average time: 35,2281946ms (s 4,12219056603083ms)
Simple LINQ average time: 7,9850022ms (s 2,60721099540673ms)

So to compare things (run 2):

Test	NH prior to the PR	NH altered by this PR	% (base is prior to the PR)
List parameter	25.28ms	22.42ms	-11.3%ms
Non parameterized constant	35.22ms	23.15ms	-34.3%ms
Simple LINQ	7.98ms	7.82ms	-2%ms

So cases optimized by the change (the two first) are actually optimized, while the case for which it can only be a penalty is not significantly impacted. (The apparent gain it even has with the PR is below sample standard deviation, so it is not significant.)

[fredericDelaporte]

Rebased.

[hazzik]

Try-catch optimization in tests looks ugly. Maybe we need to consider option to disable it for tests.

[fredericDelaporte]

Why not, but I would see that as just a nice to have.

[fredericDelaporte]

@hazzik, I think this PR should also be included in 5.1.

[hazzik]

ParameterExpander will duplicate the node again.

[fredericDelaporte]

It needs to duplicate each parameter node potentially many times, due to the possibility of having many aliases for the same parameter. So it would be awkward to change it for not duplicating by example for the last current parameter's alias while duplicating for previous ones.

Then the other option would be to avoid the duplication in Translate specifically for this case. I am not sure it is worth it. (After all the benchmark still show a gain despite this unneeded duplication for this specific case.) But I may still add a commit for looking how it looks to implement this optimization.

[fredericDelaporte]

Well it will not look very good.

We need to either add a new interface method useful only for one of the three current implementations and meaningless for the others (and incurring the bloat of using at first an extension method for avoiding a breaking change), or to cast to NhLinqExpression and use a specific method on it.

Additional commit done. All to be squashed together, or with second and/or third commit dropped if considered better not to have them.

[hazzik]

Drop last commit. It's ugly.

I still think that this can be done without need of additional clones. But I don't want to waste time now exploring these options.

[fredericDelaporte]

Dropped. (And it was not even exploiting the additional parameter...)