From 1f5a3d8d259377fb4bf4e9b2b0148a3c55cd56b1 Mon Sep 17 00:00:00 2001 From: "Uwe L. Korn" Date: Sat, 11 Oct 2014 19:09:47 +0100 Subject: [PATCH 1/7] Remove virtual keyword from some functions. These functions are not overloaded at the moment in any child class but some of them are called very often which adds a significant overhead. If really needed they should be reverted to virtual when we actually overload them. --- src/libtomahawk/playlist/PlayableProxyModel.h | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/libtomahawk/playlist/PlayableProxyModel.h b/src/libtomahawk/playlist/PlayableProxyModel.h index 6f53995ca..f4b3ee802 100644 --- a/src/libtomahawk/playlist/PlayableProxyModel.h +++ b/src/libtomahawk/playlist/PlayableProxyModel.h @@ -43,7 +43,7 @@ public: virtual QString guid() const; - virtual PlayableModel* sourceModel() const { return m_model; } + PlayableModel* sourceModel() const { return m_model; } virtual void setSourcePlayableModel( PlayableModel* sourceModel ); virtual void setSourceModel( QAbstractItemModel* model ); @@ -59,18 +59,18 @@ public: virtual void removeIndexes( const QModelIndexList& indexes ); virtual void removeIndexes( const QList< QPersistentModelIndex >& indexes ); - virtual bool showOfflineResults() const { return m_showOfflineResults; } - virtual void setShowOfflineResults( bool b ); + bool showOfflineResults() const { return m_showOfflineResults; } + void setShowOfflineResults( bool b ); - virtual bool hideDupeItems() const { return m_hideDupeItems; } - virtual void setHideDupeItems( bool b ); + bool hideDupeItems() const { return m_hideDupeItems; } + void setHideDupeItems( bool b ); - virtual int maxVisibleItems() const { return m_maxVisibleItems; } - virtual void setMaxVisibleItems( int items ); + int maxVisibleItems() const { return m_maxVisibleItems; } + void setMaxVisibleItems( int items ); - virtual PlayableItem* itemFromIndex( const QModelIndex& index ) const { return sourceModel()->itemFromIndex( index ); } - virtual PlayableItem* itemFromQuery( const Tomahawk::query_ptr& query ) const { return sourceModel()->itemFromQuery( query ); } - virtual PlayableItem* itemFromResult( const Tomahawk::result_ptr& result ) const { return sourceModel()->itemFromResult( result ); } + PlayableItem* itemFromIndex( const QModelIndex& index ) const { return sourceModel()->itemFromIndex( index ); } + PlayableItem* itemFromQuery( const Tomahawk::query_ptr& query ) const { return sourceModel()->itemFromQuery( query ); } + PlayableItem* itemFromResult( const Tomahawk::result_ptr& result ) const { return sourceModel()->itemFromResult( result ); } virtual Tomahawk::playlistinterface_ptr playlistInterface() const; void setPlaylistInterface( const Tomahawk::playlistinterface_ptr& playlistInterface ); @@ -103,7 +103,7 @@ signals: void selectRequest( const QPersistentModelIndex& index ); protected: - virtual bool filterAcceptsRow( int sourceRow, const QModelIndex& sourceParent ) const; + bool filterAcceptsRow( int sourceRow, const QModelIndex& sourceParent ) const Q_DECL_OVERRIDE; virtual bool lessThan( const QModelIndex& left, const QModelIndex& right ) const; Tomahawk::playlistinterface_ptr m_playlistInterface; From 5da17365296236e63c49ae899e41986e9db7f606 Mon Sep 17 00:00:00 2001 From: "Uwe L. Korn" Date: Sat, 11 Oct 2014 19:11:56 +0100 Subject: [PATCH 2/7] Add lazyness to filterAcceptsRow --- src/libtomahawk/playlist/PlayableProxyModel.cpp | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/libtomahawk/playlist/PlayableProxyModel.cpp b/src/libtomahawk/playlist/PlayableProxyModel.cpp index 72ca830b6..53cecb81d 100644 --- a/src/libtomahawk/playlist/PlayableProxyModel.cpp +++ b/src/libtomahawk/playlist/PlayableProxyModel.cpp @@ -145,15 +145,12 @@ PlayableProxyModel::setSourcePlayableModel( PlayableModel* sourceModel ) bool PlayableProxyModel::filterAcceptsRow( int sourceRow, const QModelIndex& sourceParent ) const { - bool dupeFilter = true; - bool visibilityFilter = true; + if ( m_hideDupeItems && !dupeFilterAcceptsRow( sourceRow, sourceParent ) ) + return false; + if ( m_maxVisibleItems > 0 && !visibilityFilterAcceptsRow( sourceRow, sourceParent ) ) + return false; - if ( m_hideDupeItems ) - dupeFilter = dupeFilterAcceptsRow( sourceRow, sourceParent ); - if ( m_maxVisibleItems > 0 ) - visibilityFilter = visibilityFilterAcceptsRow( sourceRow, sourceParent ); - - return ( dupeFilter && visibilityFilter && nameFilterAcceptsRow( sourceRow, sourceParent ) ); + return nameFilterAcceptsRow( sourceRow, sourceParent ); } From 8eab0442f27d384fd136fe0d070a174d3903d92e Mon Sep 17 00:00:00 2001 From: "Uwe L. Korn" Date: Sat, 11 Oct 2014 19:23:06 +0100 Subject: [PATCH 3/7] Only run the loop until the condition changes --- src/libtomahawk/playlist/PlayableProxyModel.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libtomahawk/playlist/PlayableProxyModel.cpp b/src/libtomahawk/playlist/PlayableProxyModel.cpp index 53cecb81d..2e9c551db 100644 --- a/src/libtomahawk/playlist/PlayableProxyModel.cpp +++ b/src/libtomahawk/playlist/PlayableProxyModel.cpp @@ -189,7 +189,7 @@ PlayableProxyModel::visibilityFilterAcceptsRow( int sourceRow, const QModelIndex return true; int items = 0; - for ( int i = 0; i < sourceRow; i++ ) + for ( int i = 0; ( i < sourceRow ) && ( items < m_maxVisibleItems ) ; i++ ) { if ( dupeFilterAcceptsRow( i, sourceParent ) && nameFilterAcceptsRow( i, sourceParent ) ) { From 44ae9f7608d32c5c0187d4d89f118ddfbd7b5e64 Mon Sep 17 00:00:00 2001 From: "Uwe L. Korn" Date: Sun, 12 Oct 2014 09:50:01 +0100 Subject: [PATCH 4/7] Less calls to itemFromIndex --- .../playlist/PlayableProxyModel.cpp | 36 ++++++++++--------- src/libtomahawk/playlist/PlayableProxyModel.h | 5 +-- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/libtomahawk/playlist/PlayableProxyModel.cpp b/src/libtomahawk/playlist/PlayableProxyModel.cpp index 2e9c551db..6fe5cd5af 100644 --- a/src/libtomahawk/playlist/PlayableProxyModel.cpp +++ b/src/libtomahawk/playlist/PlayableProxyModel.cpp @@ -145,25 +145,32 @@ PlayableProxyModel::setSourcePlayableModel( PlayableModel* sourceModel ) bool PlayableProxyModel::filterAcceptsRow( int sourceRow, const QModelIndex& sourceParent ) const { - if ( m_hideDupeItems && !dupeFilterAcceptsRow( sourceRow, sourceParent ) ) - return false; - if ( m_maxVisibleItems > 0 && !visibilityFilterAcceptsRow( sourceRow, sourceParent ) ) + PlayableItem* pi = itemFromIndex( sourceModel()->index( sourceRow, 0, sourceParent ) ); + if ( !pi ) return false; - return nameFilterAcceptsRow( sourceRow, sourceParent ); + return filterAcceptsRowInternal( sourceRow, pi, sourceParent ); } bool -PlayableProxyModel::dupeFilterAcceptsRow( int sourceRow, const QModelIndex& sourceParent ) const +PlayableProxyModel::filterAcceptsRowInternal( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent ) const +{ + if ( m_hideDupeItems && !dupeFilterAcceptsRow( sourceRow, pi, sourceParent ) ) + return false; + if ( m_maxVisibleItems > 0 && !visibilityFilterAcceptsRow( sourceRow, sourceParent ) ) + return false; + + return nameFilterAcceptsRow( sourceRow, pi, sourceParent ); +} + + +bool +PlayableProxyModel::dupeFilterAcceptsRow( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent ) const { if ( !m_hideDupeItems ) return true; - PlayableItem* pi = itemFromIndex( sourceModel()->index( sourceRow, 0, sourceParent ) ); - if ( !pi ) - return false; - for ( int i = 0; i < sourceRow; i++ ) { PlayableItem* di = itemFromIndex( sourceModel()->index( i, 0, sourceParent ) ); @@ -174,7 +181,7 @@ PlayableProxyModel::dupeFilterAcceptsRow( int sourceRow, const QModelIndex& sour ( pi->album() && pi->album() == di->album() ) || ( pi->artist() && pi->artist()->name() == di->artist()->name() ); - if ( b && filterAcceptsRow( i, sourceParent ) ) + if ( b && filterAcceptsRowInternal( i, di, sourceParent ) ) return false; } @@ -191,7 +198,8 @@ PlayableProxyModel::visibilityFilterAcceptsRow( int sourceRow, const QModelIndex int items = 0; for ( int i = 0; ( i < sourceRow ) && ( items < m_maxVisibleItems ) ; i++ ) { - if ( dupeFilterAcceptsRow( i, sourceParent ) && nameFilterAcceptsRow( i, sourceParent ) ) + PlayableItem* pi = itemFromIndex( sourceModel()->index( i, 0, sourceParent ) ); + if ( pi && dupeFilterAcceptsRow( i, pi, sourceParent ) && nameFilterAcceptsRow( i, pi, sourceParent ) ) { items++; } @@ -202,12 +210,8 @@ PlayableProxyModel::visibilityFilterAcceptsRow( int sourceRow, const QModelIndex bool -PlayableProxyModel::nameFilterAcceptsRow( int sourceRow, const QModelIndex& sourceParent ) const +PlayableProxyModel::nameFilterAcceptsRow( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent ) const { - PlayableItem* pi = itemFromIndex( sourceModel()->index( sourceRow, 0, sourceParent ) ); - if ( !pi ) - return false; - if ( m_hideEmptyParents && pi->source() ) { if ( !sourceModel()->rowCount( sourceModel()->index( sourceRow, 0, sourceParent ) ) ) diff --git a/src/libtomahawk/playlist/PlayableProxyModel.h b/src/libtomahawk/playlist/PlayableProxyModel.h index f4b3ee802..9adc0aa93 100644 --- a/src/libtomahawk/playlist/PlayableProxyModel.h +++ b/src/libtomahawk/playlist/PlayableProxyModel.h @@ -117,8 +117,9 @@ private slots: void onCurrentIndexChanged( const QModelIndex& newIndex, const QModelIndex& oldIndex ); private: - bool nameFilterAcceptsRow( int sourceRow, const QModelIndex& sourceParent ) const; - bool dupeFilterAcceptsRow( int sourceRow, const QModelIndex& sourceParent ) const; + bool filterAcceptsRowInternal( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent ) const; + bool nameFilterAcceptsRow( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent ) const; + bool dupeFilterAcceptsRow( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent ) const; bool visibilityFilterAcceptsRow( int sourceRow, const QModelIndex& sourceParent ) const; bool lessThan( int column, const Tomahawk::query_ptr& left, const Tomahawk::query_ptr& right ) const; From b970cf14332b7f2c5e1c65c9f854c39547dec3a4 Mon Sep 17 00:00:00 2001 From: "Uwe L. Korn" Date: Sun, 12 Oct 2014 09:54:46 +0100 Subject: [PATCH 5/7] First do visibilty check to reduce branching --- src/libtomahawk/playlist/PlayableProxyModel.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libtomahawk/playlist/PlayableProxyModel.cpp b/src/libtomahawk/playlist/PlayableProxyModel.cpp index 6fe5cd5af..88d521d86 100644 --- a/src/libtomahawk/playlist/PlayableProxyModel.cpp +++ b/src/libtomahawk/playlist/PlayableProxyModel.cpp @@ -156,10 +156,10 @@ PlayableProxyModel::filterAcceptsRow( int sourceRow, const QModelIndex& sourcePa bool PlayableProxyModel::filterAcceptsRowInternal( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent ) const { - if ( m_hideDupeItems && !dupeFilterAcceptsRow( sourceRow, pi, sourceParent ) ) - return false; if ( m_maxVisibleItems > 0 && !visibilityFilterAcceptsRow( sourceRow, sourceParent ) ) return false; + if ( m_hideDupeItems && !dupeFilterAcceptsRow( sourceRow, pi, sourceParent ) ) + return false; return nameFilterAcceptsRow( sourceRow, pi, sourceParent ); } From f85e34a3e99038d3fd2c939f3bf7355f4f2df4a3 Mon Sep 17 00:00:00 2001 From: "Uwe L. Korn" Date: Sun, 12 Oct 2014 11:29:44 +0100 Subject: [PATCH 6/7] Memoize visibility filter --- .../playlist/PlayableProxyModel.cpp | 30 ++++++++++++------- src/libtomahawk/playlist/PlayableProxyModel.h | 20 +++++++++++-- 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/libtomahawk/playlist/PlayableProxyModel.cpp b/src/libtomahawk/playlist/PlayableProxyModel.cpp index 88d521d86..e58aafc00 100644 --- a/src/libtomahawk/playlist/PlayableProxyModel.cpp +++ b/src/libtomahawk/playlist/PlayableProxyModel.cpp @@ -145,20 +145,22 @@ PlayableProxyModel::setSourcePlayableModel( PlayableModel* sourceModel ) bool PlayableProxyModel::filterAcceptsRow( int sourceRow, const QModelIndex& sourceParent ) const { + PlayableProxyModelFilterMemo memo; + PlayableItem* pi = itemFromIndex( sourceModel()->index( sourceRow, 0, sourceParent ) ); if ( !pi ) return false; - return filterAcceptsRowInternal( sourceRow, pi, sourceParent ); + return filterAcceptsRowInternal( sourceRow, pi, sourceParent, memo ); } bool -PlayableProxyModel::filterAcceptsRowInternal( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent ) const +PlayableProxyModel::filterAcceptsRowInternal( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent, PlayableProxyModelFilterMemo& memo ) const { - if ( m_maxVisibleItems > 0 && !visibilityFilterAcceptsRow( sourceRow, sourceParent ) ) + if ( m_maxVisibleItems > 0 && !visibilityFilterAcceptsRow( sourceRow, sourceParent, memo ) ) return false; - if ( m_hideDupeItems && !dupeFilterAcceptsRow( sourceRow, pi, sourceParent ) ) + if ( m_hideDupeItems && !dupeFilterAcceptsRow( sourceRow, pi, sourceParent, memo ) ) return false; return nameFilterAcceptsRow( sourceRow, pi, sourceParent ); @@ -166,7 +168,7 @@ PlayableProxyModel::filterAcceptsRowInternal( int sourceRow, PlayableItem* pi, c bool -PlayableProxyModel::dupeFilterAcceptsRow( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent ) const +PlayableProxyModel::dupeFilterAcceptsRow( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent, PlayableProxyModelFilterMemo& memo ) const { if ( !m_hideDupeItems ) return true; @@ -181,7 +183,7 @@ PlayableProxyModel::dupeFilterAcceptsRow( int sourceRow, PlayableItem* pi, const ( pi->album() && pi->album() == di->album() ) || ( pi->artist() && pi->artist()->name() == di->artist()->name() ); - if ( b && filterAcceptsRowInternal( i, di, sourceParent ) ) + if ( b && filterAcceptsRowInternal( i, di, sourceParent, memo ) ) return false; } @@ -190,18 +192,26 @@ PlayableProxyModel::dupeFilterAcceptsRow( int sourceRow, PlayableItem* pi, const bool -PlayableProxyModel::visibilityFilterAcceptsRow( int sourceRow, const QModelIndex& sourceParent ) const +PlayableProxyModel::visibilityFilterAcceptsRow( int sourceRow, const QModelIndex& sourceParent, PlayableProxyModelFilterMemo& memo ) const { if ( m_maxVisibleItems <= 0 ) return true; - int items = 0; - for ( int i = 0; ( i < sourceRow ) && ( items < m_maxVisibleItems ) ; i++ ) + if ( static_cast( sourceRow ) < memo.visibilty.size() ) + { + // We have already memoized the return value. + return memo.visibilty[sourceRow] < m_maxVisibleItems; + } + + int items = memo.visibilty.back(); + for ( int i = memo.visibilty.size() - 1; ( i < sourceRow ) && ( items < m_maxVisibleItems ) ; i++ ) { PlayableItem* pi = itemFromIndex( sourceModel()->index( i, 0, sourceParent ) ); - if ( pi && dupeFilterAcceptsRow( i, pi, sourceParent ) && nameFilterAcceptsRow( i, pi, sourceParent ) ) + // We will not change memo in these calls as all values needed in them are already memoized. + if ( pi && dupeFilterAcceptsRow( i, pi, sourceParent, memo ) && nameFilterAcceptsRow( i, pi, sourceParent ) ) { items++; + memo.visibilty.push_back( items ); // Sets memo.visibilty[i + 1] to items } } diff --git a/src/libtomahawk/playlist/PlayableProxyModel.h b/src/libtomahawk/playlist/PlayableProxyModel.h index 9adc0aa93..86a92dceb 100644 --- a/src/libtomahawk/playlist/PlayableProxyModel.h +++ b/src/libtomahawk/playlist/PlayableProxyModel.h @@ -27,6 +27,20 @@ #include "DllMacro.h" +class PlayableProxyModelFilterMemo +{ +public: + PlayableProxyModelFilterMemo() + { + // First element always has no predecessors. + // TODO C++11: Make this a constexpr using initializer lists. + visibilty.push_back( 0 ); + } + + virtual ~PlayableProxyModelFilterMemo() {} + std::vector visibilty; +}; + class DLLEXPORT PlayableProxyModel : public QSortFilterProxyModel { Q_OBJECT @@ -117,10 +131,10 @@ private slots: void onCurrentIndexChanged( const QModelIndex& newIndex, const QModelIndex& oldIndex ); private: - bool filterAcceptsRowInternal( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent ) const; + bool filterAcceptsRowInternal( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent, PlayableProxyModelFilterMemo& memo ) const; bool nameFilterAcceptsRow( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent ) const; - bool dupeFilterAcceptsRow( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent ) const; - bool visibilityFilterAcceptsRow( int sourceRow, const QModelIndex& sourceParent ) const; + bool dupeFilterAcceptsRow( int sourceRow, PlayableItem* pi, const QModelIndex& sourceParent, PlayableProxyModelFilterMemo& memo ) const; + bool visibilityFilterAcceptsRow( int sourceRow, const QModelIndex& sourceParent, PlayableProxyModelFilterMemo& memo ) const; bool lessThan( int column, const Tomahawk::query_ptr& left, const Tomahawk::query_ptr& right ) const; PlayableModel* m_model; From f6503aba081bba60b018652146d5ef615c4d427d Mon Sep 17 00:00:00 2001 From: "Uwe L. Korn" Date: Sun, 12 Oct 2014 12:54:18 +0100 Subject: [PATCH 7/7] Various small performance improvements --- .../playlist/PlayableProxyModel.cpp | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/libtomahawk/playlist/PlayableProxyModel.cpp b/src/libtomahawk/playlist/PlayableProxyModel.cpp index e58aafc00..036bc5dc3 100644 --- a/src/libtomahawk/playlist/PlayableProxyModel.cpp +++ b/src/libtomahawk/playlist/PlayableProxyModel.cpp @@ -230,25 +230,27 @@ PlayableProxyModel::nameFilterAcceptsRow( int sourceRow, PlayableItem* pi, const } } - if ( pi->query() ) + const Tomahawk::query_ptr& query = pi->query(); + if ( query ) { Tomahawk::result_ptr r; - if ( pi->query()->numResults() ) - r = pi->query()->results().first(); + if ( query->numResults() ) + r = query->results().first(); if ( !m_showOfflineResults && ( r.isNull() || !r->isOnline() ) ) return false; - if ( filterRegExp().isEmpty() ) + const QRegExp regexp = filterRegExp(); + if ( regexp.isEmpty() ) return true; - QStringList sl = filterRegExp().pattern().split( " ", QString::SkipEmptyParts ); - foreach( QString s, sl ) + QStringList sl = regexp.pattern().split( " ", QString::SkipEmptyParts ); + foreach( const QString& s, sl ) { - s = s.toLower(); - if ( !pi->query()->track()->artist().toLower().contains( s ) && - !pi->query()->track()->album().toLower().contains( s ) && - !pi->query()->track()->track().toLower().contains( s ) ) + const Tomahawk::track_ptr& track = query->track(); + if ( !track->artist().contains( s, Qt::CaseInsensitive ) && + !track->album().contains( s, Qt::CaseInsensitive ) && + !track->track().contains( s, Qt::CaseInsensitive ) ) { return false; } @@ -263,7 +265,8 @@ PlayableProxyModel::nameFilterAcceptsRow( int sourceRow, PlayableItem* pi, const bool found = true; foreach( const QString& s, sl ) { - if ( !al->name().contains( s, Qt::CaseInsensitive ) && !al->artist()->name().contains( s, Qt::CaseInsensitive ) ) + if ( !al->name().contains( s, Qt::CaseInsensitive ) && + !al->artist()->name().contains( s, Qt::CaseInsensitive ) ) { found = false; } @@ -280,7 +283,8 @@ PlayableProxyModel::nameFilterAcceptsRow( int sourceRow, PlayableItem* pi, const bool found = true; foreach( const QString& s, sl ) { - if ( !ar->name().contains( s, Qt::CaseInsensitive ) && !ar->artist()->name().contains( s, Qt::CaseInsensitive ) ) + if ( !ar->name().contains( s, Qt::CaseInsensitive ) && + !ar->artist()->name().contains( s, Qt::CaseInsensitive ) ) { found = false; }