Avoid re-using query plan embedding different constant values. #1542
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fredericDelaporte
force-pushed
the
NH-2658
branch
from
23ddb4d to
b681c58
Compare
* Fixes nhibernate#1330 (NH-3673) * Obsoletes nhibernate#866 (NH-2658) * Fixes most of nhibernate#1363 (a workaround is still needed for one case; NH-2500)
fredericDelaporte
force-pushed
the
NH-2658
branch
from
b681c58 to
58e74d5
Compare
| plan = new QueryExpressionPlan(queryExpression, shallow, enabledFilters, factory); | ||
| planCache.Put(key, plan); | ||
|
|
||
| if (wasAlreadyRefined == false && refinableQuery.RefinedKey) |
There was a problem hiding this comment.
Here, instead of caching with original and refined key, we could take quite a different path: cache nothing. So the RefinedKey would be renamed NotCacheable, it would be checked right after plan generation, and if true, the plan would not be put in cache. It would have several benefits:
- Simpler.
- Would solve last NH-2500 - NH 3.0 Linq provider uses query parameters from first call in subsequent calls. #1363 trouble without the workaround.
- Would avoid retaining captured object instances.
- Would avoid bloating the cache with as many entries as constant values used with queries having non-parameterized constants.
In #1363, some users were even asking for a way to fully disable the query plan cache, writing they were not feeling a performance hit for not having it. Here it would disable it only for queries embedding constants which are not translated to HQL parameters, so it will be even less likely to have a negative impact.
Update: now proposed in #1544.
|
Replaced by #1544. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As said in #1363, I find the
insideSelectClausetrick a bit hackish, and I was willing to investigate patches from #1363 and #866 corresponding JIRAs.The one from #1363 was quite interesting, but is missing cases where the constant is not parameterized while not being in select clause (handled by a post transformer), as in #866. (Cases which are also missed by the
insideSelectClausetrick.)Patch from #866 just addresses custom LINQ extensions needing to signal a constant as non-parameterized. So it was fixing just a very small subset of the trouble, while requiring many changes, both in NHibernate and on user side.
Here I propose yet another way: detect than some constants have not been used as parameters, and refine adequately the plan caching. This must be a two steps process since such constants are only known once the plan has been generated.
So at that point, if such constants are detected, the query is flagged as refined, it has a new refined key, and it is cached for that additional key, along with being cached with its "original" key.
On cache hit with a non-refined key, the key is refined thanks to plan's query informations, and checked against plan's query again. If not matching, it is looked-up again, but this time with the key already refined.
It allows to remove the
insideSelectClausetrick, and solves most of the cases. Only the "cached object instance" issue remains, but with a workaround.If this PR is merged before #1540, #1540 would then be obsoleted and no more needed.