[pvr] optimize channel groups loading/manipulation by avoiding unnecessary ... #1982

Merged
merged 1 commit into from Dec 30, 2012

Conversation

Projects
None yet
2 participants
Contributor

nemphys commented Dec 24, 2012

... sorting and renumbering calls

@opdenkamp opdenkamp and 1 other commented on an outdated diff Dec 24, 2012

xbmc/addons/AddonCallbacksPVR.cpp
@@ -130,7 +130,9 @@ void CAddonCallbacksPVR::PVRTransferChannelGroupMember(void *addonData, const AD
else if (group->IsRadio() == channel->IsRadio())
{
/* transfer this entry to the group */
- group->AddToGroup(*channel, member->iChannelNumber, false);
+ group->SetPreventSortAndRenumber();
@opdenkamp

opdenkamp Dec 24, 2012

Member

i'd rather see you handle this in CPVRChannelGroup when updating

@nemphys

nemphys Dec 24, 2012

Contributor

Actually, checking the code I realized this was the only instance AddToGroup was called with bSortAndRenumber set to false, so I thought there was some reason for this.
That's why I kept it that way

@nemphys

nemphys Dec 24, 2012

Contributor

When is this method called? Actually, since I am away and cannot test it right now, I am worried this might conflict with the purpose of this whole thing (eg. if it is called for the dummy group, too).
Please advise

@opdenkamp

opdenkamp Dec 24, 2012

Member

it's called by the add-on when updating group members in a group

@nemphys

nemphys Dec 24, 2012

Contributor

OK, so I think this logic should be completely removed from here and add a SetPreventSortAndupdate() call to the dummy group in CPVRChannelGroup::Update, too (missed that). If I understand correctly, this is when this method here is called (as a callback, of course)

@nemphys

nemphys Dec 24, 2012

Contributor

Please advise, so that I can update the PR

Contributor

nemphys commented Dec 24, 2012

Also, I think we need to do some work and revisit all calls to sorting/renumbering methods, as I see a lot of calls everywhere (maybe make them more centralized somehow?). For instance, SortAndRenumber does the sorting first and then calls Renumber, which calls SortByChannelNumber again before it finishes. Is there a reason for that or should I just change it to Renumber first and then sort only once?

Contributor

nemphys commented Dec 24, 2012

After inspected further, concerning my last comment, I think we should remove the sorting call from the Renumber method (since it didn't even respect the m_bUsingBackendChannelOrder setting), call it first in SortAndRenumber and then do the sorting. This will save us a sorting call (and correct the setting ignoring behavior).
What do you think?
ps. this means SortAndRenumber should be changed to RenumberAndSort, too, but I don't think that's too urgent right now.

Member

opdenkamp commented Dec 24, 2012

I think it's time for christmas diner :)

On 12/24/2012 03:31 PM, Dimitris Kazakos wrote:

After inspected further, concerning my last comment, I think we should
remove the sorting call from the Renumber method (since it didn't even
respect the m_bUsingBackendChannelOrder setting), call it first in
SortAndRenumber and then do the sorting. This will save us a sorting
call (and correct the setting ignoring behavior).
What do you think?


Reply to this email directly or view it on GitHub
#1982 (comment).

Contributor

nemphys commented Dec 24, 2012

A little early for me here in Greece, will update the PR and you can take a look at it when you can :-)
Merry Xmas!

@opdenkamp opdenkamp and 1 other commented on an outdated diff Dec 28, 2012

xbmc/pvr/channels/PVRChannelGroup.cpp
@@ -314,14 +321,20 @@ bool CPVRChannelGroup::SortAndRenumber(void)
void CPVRChannelGroup::SortByClientChannelNumber(void)
{
- CSingleLock lock(m_critSection);
- sort(m_members.begin(), m_members.end(), sortByClientChannelNumber());
+ if (!PreventSortAndRenumber())
+ {
+ CSingleLock lock(m_critSection);
@opdenkamp

opdenkamp Dec 28, 2012

Member

should be above the condition

@nemphys

nemphys Dec 28, 2012

Contributor

Right, or I can change the getter method to lock before reading the value so that it is more robust.
What do you prefer?

@opdenkamp

opdenkamp Dec 29, 2012

Member

lock before the condition so it's locked while being sorted

@nemphys

nemphys Dec 29, 2012

Contributor

I changed it so that PreventSortAndRenumber() locks to read the value. Once it's checked, there is a lock, too.
So, while the sorting happens, it is locked. What do you mean?

@opdenkamp

opdenkamp Dec 29, 2012

Member

and what if another thread accesses it between the two locks? just put it two lines up

@nemphys

nemphys Dec 29, 2012

Contributor

done

@opdenkamp opdenkamp commented on an outdated diff Dec 28, 2012

xbmc/pvr/channels/PVRChannelGroup.cpp
}
void CPVRChannelGroup::SortByChannelNumber(void)
{
- CSingleLock lock(m_critSection);
- sort(m_members.begin(), m_members.end(), sortByChannelNumber());
+ if (!PreventSortAndRenumber())
+ {
+ CSingleLock lock(m_critSection);
@opdenkamp

opdenkamp Dec 28, 2012

Member

same here

@opdenkamp opdenkamp and 1 other commented on an outdated diff Dec 28, 2012

xbmc/pvr/channels/PVRChannelGroup.cpp
@@ -612,6 +627,8 @@ bool CPVRChannelGroup::AddAndUpdateChannels(const CPVRChannelGroup &channels, bo
}
}
+ SetPreventSortAndRenumber(false);
@opdenkamp

opdenkamp Dec 28, 2012

Member

you need to sort the group after the import

@nemphys

nemphys Dec 28, 2012

Contributor

Yes, I will add a call to SortAndRenumber right here

@nemphys

nemphys Dec 28, 2012

Contributor

...and revert m_bSortAndRenumber to its previous value instead of false...

Contributor

nemphys commented Dec 28, 2012

PR updated, please check

Contributor

nemphys commented Dec 29, 2012

PR updated again, please check

@opdenkamp opdenkamp added a commit that referenced this pull request Dec 30, 2012

@opdenkamp opdenkamp Merge pull request #1982 from nemphys/sortandrenumber
[pvr] optimize channel groups loading/manipulation by avoiding unnecessary ...
1cc8361

@opdenkamp opdenkamp merged commit 1cc8361 into xbmc:master Dec 30, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment