-
-
Notifications
You must be signed in to change notification settings - Fork 110
Referenced cache performance #92
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
Conversation
2fcb211 to
35870aa
Compare
|
ping @hrach - refactor to more clean version |
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.
Place $this->refCache['referenced'] = []; here.
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.
running line $this->refCache['referenced'] = []; for GroupedSelection was actually the problem that i am trying to solve. This will actually break the query.count.phpt test
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.
Why?
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.
Sorry, my previous comment, which I deleted yesterday?, was wrong. I misread the code. The previous version was probably better :| I really sorry.
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.
Because of the iteration in iteration. The deleting referencedCache is launched for each inner interation. This leads to increasing number of sent queries (from 3 to 5 in query.count.phpt)
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.
I don't see any difference between your code and this solution:
class GroupedSelection
protected function emptyResultSet(..)
{
parent::emptyResultSet($saveCache);
$this->refCache['referenced'] = [];
}
Where is a difference?
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.
Difference:
- My solution:
WhenemptyResultSet()is called on Selection, the$this->refCache['referenced'] = [];line is executed. (the default value of second parameter is true)
When emptyResultSet() is called on GroupedSelection, the $this->refCache['referenced'] = []; is not executed (the second parameter of emptyResultSet is false)
- Your solution
Its actually the opposite version of mine.
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.
to @hrach no problem :) i kind of prefer the previous version too. I was actually quite thinking about the realisation of what you thought
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.
Oh, now I see. So what about
$old = $this->refCache['referenced'];
parent::emptyResultSet($saveCache);
$this->refCache['referenced'] = $old;
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.
The reason of adding a parameter was that i was thinking e.g. about adding new descendant of Selection. Than this behaviour will be probably shared so i choose a parameter.
But i am completely fine with your solution too, if this is not an issue.
…roupedSelection), performance improvment
35870aa to
589ff75
Compare
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.
shoud be FALSE
Selection: referenced cache cleared only for root selection (not in GroupedSelection), performance improvment
solves #91