[PVR] Improve performance of TriggerRecordingUpdate/TriggerTimerUpdate

[djp952]

Description

While doing some performance testing on my PVR addon, I noticed that TriggerRecordingUpdate() was taking longer than seemed necessary. Digging into it a bit, I found that the data provided by the PVR addon was being duplicated unnecessarily by deep-copying a shared_ptr<> instance that has no need to be deep-copied.

I think this type of optimization may ultimately also be applicable to other PVR transfers (EPG, Channel Groups, Channels, Timers), but would prefer to see how this PR goes first before recommending any additional changes. Each transfer function is different and may or may not benefit in the same way.

edit: The same type of change only affected EPG and Timers. EPG is in the process of being refactored; so I only added a change for Timers.

Motivation and Context

A measurable (> 40%) performance improvement transferring PVR_RECORDING information from a PVR addon to Kodi. There was little to no associated improvement for the PVR_TIMER information as there will typically be a relatively small number of these.

How Has This Been Tested?

Tested on Windows (Desktop) x64, using recent Kodi 19 "Matrix" nightly build. No functionality issues were detected. I compared the state of the original shared_ptr<> with the deep-copied one and found no discrepancies. Performance increase (less addon overhead) for transferring 922 recording structures dropped from 171ms to 93ms, which is a 45% improvement.

I did not test this PR on Leia branch, and am of the opinion that even if accepted for master branch it's not worth a back-port since no defects have been addressed.

If accepted for master branch, I would be willing to look at the other PVR transfer operations to see if they may benefit some similar optimizations.

Screenshots (if appropriate):

N/A

Types of change

• Bug fix (non-breaking change which fixes an issue)
• Clean up (non-breaking change which removes non-working, unmaintained functionality)
• Improvement (non-breaking change which improves existing functionality)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that will cause existing functionality to change)
• Cosmetic change (non-breaking change that doesn't touch code)
• None of the above (please explain below)

Checklist:

• My code follows the Code Guidelines of this project
• My change requires a change to the documentation, either Doxygen or wiki
• I have updated the documentation accordingly
• I have read the Contributing document
• I have added tests to cover my change
• All new and existing tests passed

Review by @ksooo is requested if PR builds successfully for all supported platforms.

[ksooo]

Very nice. This logics was there before I joined the project and I never took a closer look. Thanks.

[ksooo]

If accepted for master branch, I would be willing to look at the other PVR transfer operations to see if they may benefit some similar optimizations.

That would be great.

EDIT: Please don't touch EPG as I'm currently doing major changes in this area.

[djp952]

Understood. Simplification made to remove std::make_pair() and squashed.

EPG actually benefits a lot time-wise for operations like right after a "Clear Data" and everything is reloaded by avoiding the deep copy. Here is what I had for that (hasn't tested yet, just did it), will discard and omit from any subsequent PR. Thank you @ksooo!

bool CPVREpg::UpdateEntry(const std::shared_ptr<CPVREpgInfoTag>& tag, bool bUpdateDatabase)
{
  std::shared_ptr<CPVREpgInfoTag> infoTag;

  CSingleLock lock(m_critSection);
  const auto it = m_tags.find(tag->StartAsUTC());
  if (it != m_tags.end())
  {
    infoTag = it->second;
    infoTag->Update(*tag, false);
  }
  else
  {
    infoTag = tag;
    m_tags.insert({tag->StartAsUTC(), infoTag});
  }

  infoTag->SetChannelData(m_channelData);
  infoTag->SetEpgID(m_iEpgID);

  if (bUpdateDatabase)
    m_changedTags.insert({infoTag->UniqueBroadcastID(), infoTag});

  return true;
}

[djp952]

Added suggested changes for PVR::TransferTimerEntry(), as it had the same basic problem. The addition is expected to be negligible time-wise and not be noticeable to an end-user. The remaining non-EPG functions (channels, channel groups, channel group members) are implemented differently and more efficiently.

Since EPG was omitted and only Timers came up, I opted to collapse that change into the same PR. LMK if that is not acceptable and I can break them up.

[ksooo]

Perfect. Thank you.

[ksooo]

@djp952 this introduces a small regression with recordings. We must ensure that the constructor that creates the recording we reuse in CPVRRecording::UpdateEntry does the same as CPVRRecording::Update does.

void CPVRRecording::Update(const CPVRRecording& tag) calls CPVRRecording::UpdatePath(); which sets m_strFileNameAndPath, but CPVRRecording::CPVRRecording(const PVR_RECORDING& recording, unsigned int iClientId) does not do this! This is a long standing bug, that hit the surface now that we merged this PR.

So, could you please open a PR that inserts the missing call to CPVRRecording::UpdatePath(); at the end of CPVRRecording::CPVRRecording(const PVR_RECORDING& recording, unsigned int iClientId) to fix the regression? Thanks.

[djp952]

Will do. Thank you for noticing the problem.