-
Notifications
You must be signed in to change notification settings - Fork 924
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
Fix for HQL query plan regenerate problem #3069 #3168
base: master
Are you sure you want to change the base?
Conversation
Many failed tests... Looks like Arithmetic Operations should be removed from |
Hi @bahusoid @hazzik, We also cannot figure out why QueryPlanCache.GetHqlQueryPlan called twice(expanded queries) and takes 7.23ms, DefaultQueryProvider.ExecuteList takes 9.9ms, it's not acceptable. Now I am stuck with failing test, I cannot go furher what could we do?
Postgresql Current test results are: We are using Dynatrace to monitor system and find bottle-necks, above image taken from there. |
From simple change it's converted to something else. Let's focus on simple fix in this PR and expression types that require additional handling be moved in separate PR. |
I have spent quite time to handle most of issues, I agree its not simple fix but you know that we have to fix this kind of performance issues. We are ready to contribute. Please consider your judment and guide us how to fix remaining issues. by the way @cokalyoncu is my colleague |
I think blocking issue with this change is `ExpandedQueryExpression' because creates multiple QueryPlanCache for every parameter with type of array,collection,list. Its bigger issue than we realize. @bahusoid For example if you have a query like this, 'guids.Contains(x.Id) && requestDates.Contains(x.RequestDate)'
Total generated query plan is 3 for just one query that has several list parameters. Above scenario shows that how its in-effective for collection parameters I think for collection parameters parameter naming and setting value must be just before command execute. Just like filters NHibernate uses 'FilterHelper.ExpandDynamicFilterParameters' to handle this. I have opened several issues past years about query plan caching let we make it better, but above scenario is complex and I don't have a clue how to do it. Pros removing ExpandedQueryExpression
Same perspective you already did with #2959 |
@maca88 already did better fix things we are talking about. #2375 #2079 @maca88 What dou you think about below issue?
|
@maca88 @hazzik @bahusoid @fredericDelaporte Why no one interesting about this issue, it’s currently hurting performance very bad already focused time to time ? #2375 #2079 implemented @maca88 about 2 years ago. A lot of folks reported these issues including us. |
It seems to me part of the reason is written here:
|
cache.Clear(); | ||
|
||
var input = new { Name = "ALFKI" }; | ||
(from c in db.Customers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gokhanabatay for all these queries you should check for the results returned. Otherwise it might be that not only plan is cached but also results are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me part of the reason is written here:
From simple change it's converted to something else. Let's focus on simple fix in this PR and expression types that require additional handling be moved in separate PR.
I cannot understand, if there is major issues so major changes needs to be applied. We want to fix these problems that we are reporting past 3 years mostly linq performance issues.
@hazzik @fredericDelaporte
Maca just made good improvements my question is what are we waiting, we are volunteered if you guide us we can make necessary changes to merge these pull requests. #2079 and #2375.
Because our changes are much simpler then maca's, we were repeating its changes without knowing its already done.
https://github.com/nhibernate/nhibernate-core/blob/04e40931af0d812810ef176deb452847154b6ee9/src/NHibernate/Linq/Visitors/SelectClauseNominator.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me part of the reason is written here:
From simple change it's converted to something else. Let's focus on simple fix in this PR and expression types that require additional handling be moved in separate PR.
I cannot understand, if there is major issues so major changes needs to be applied. We want to fix these problems that we are reporting past 3 years mostly linq performance issues.
@hazzik @fredericDelaporte Maca just made good improvements my question is what are we waiting, we are volunteered if you guide us we can make necessary changes to merge these pull requests. #2079 and #2375.
Because our changes are much simpler then maca's, we were repeating its changes without knowing its already done. https://github.com/nhibernate/nhibernate-core/blob/04e40931af0d812810ef176deb452847154b6ee9/src/NHibernate/Linq/Visitors/SelectClauseNominator.cs
@maca88 could you please tell us, what are the necessary changes your pull request neeeds to merge. Or dou you have a plan to work on this. We are talking these issues with you 2019 to today. #2079 and #2375.
These 2 PRs are 80 changed files each. They need a very careful review and debug before anyone really could assess and verify them. But we're all volunteers here and are giving our support to NHibernate in our free time (plus some out of pocket expenses on top). Basically - the rule of thumb - the smaller less invasive changes the faster PR get merged. Below are the summary of changes that needs to be done for the PRs:
|
Hi @hazzik thank you for your response, I want to help but, I am not feeling comfortable to work @maca88's changes they are too much compilcated for me. Changes in this our pr is a the tip of the iceberg better fix already done in @maca88 prs I think. Current linq provider has realy bad performance issues mentioned above and that will be resolved with @maca88 changes.
I could help in testing and necessary debugging if you lead me, also we are using nh dev builds if anything is broken after a merge we could identify it. It will be great improvement for linq nhibernate user, I kindly ask to you would you prioritize them. |
Fixes #3069