From 7b57a0c9cc1ab2002a1e85714e2a1676a09ec4ec Mon Sep 17 00:00:00 2001 From: Alexander Hirsch Date: Fri, 15 Jun 2018 12:57:16 +0200 Subject: [PATCH 1/6] Remove unnecessary bounds-checks The line numbers will be int::max() if the iterators are at the end, so we don't need to check the iterators here. --- src/data/logfiltereddata.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/data/logfiltereddata.cpp b/src/data/logfiltereddata.cpp index f99f43afd..b2670fc87 100644 --- a/src/data/logfiltereddata.cpp +++ b/src/data/logfiltereddata.cpp @@ -461,16 +461,14 @@ void LogFilteredData::regenerateFilteredItemsCache() const if ( next_mark <= next_match ) { // LOG(logDEBUG) << "Add mark at " << next_mark; filteredItemsCache_.push_back( FilteredItem( next_mark, Mark ) ); - if ( j != marks_.end() ) - ++j; - if ( ( next_mark == next_match ) && ( i != matching_lines_.cend() ) ) + ++j; + if ( ( next_mark == next_match ) ) ++i; // Case when it's both match and mark. } else { // LOG(logDEBUG) << "Add match at " << next_match; filteredItemsCache_.push_back( FilteredItem( next_match, Match ) ); - if ( i != matching_lines_.cend() ) - ++i; + ++i; } } From e64e69ccc678e513eb783c585d8eb45cb0084d69 Mon Sep 17 00:00:00 2001 From: Alexander Hirsch Date: Fri, 15 Jun 2018 12:56:36 +0200 Subject: [PATCH 2/6] Use binary flags for FilteredItemType --- src/crawlerwidget.cpp | 2 +- src/data/logfiltereddata.cpp | 14 ++++++++------ src/data/logfiltereddata.h | 32 ++++++++++++++++++++++++++++++-- src/filteredview.cpp | 2 +- src/overview.cpp | 2 +- 5 files changed, 41 insertions(+), 11 deletions(-) diff --git a/src/crawlerwidget.cpp b/src/crawlerwidget.cpp index 657d1cd1f..9eccb9596 100644 --- a/src/crawlerwidget.cpp +++ b/src/crawlerwidget.cpp @@ -386,7 +386,7 @@ void CrawlerWidget::markLineFromFiltered( qint64 line ) if ( line < logFilteredData_->getNbLine() ) { qint64 line_in_file = logFilteredData_->getMatchingLineNumber( line ); if ( logFilteredData_->filteredLineTypeByIndex( line ) - == LogFilteredData::Mark ) + & LogFilteredData::Mark ) logFilteredData_->deleteMark( line_in_file ); else logFilteredData_->addMark( line_in_file ); diff --git a/src/data/logfiltereddata.cpp b/src/data/logfiltereddata.cpp index b2670fc87..2a9335ac4 100644 --- a/src/data/logfiltereddata.cpp +++ b/src/data/logfiltereddata.cpp @@ -457,19 +457,21 @@ void LogFilteredData::regenerateFilteredItemsCache() const ( j != marks_.end() ) ? j->lineNumber() : std::numeric_limits::max(); qint64 next_match = ( i != matching_lines_.cend() ) ? i->lineNumber() : std::numeric_limits::max(); - // We choose a Mark over a Match if a line is both, just an arbitrary choice really. + FilteredLineType type = static_cast( 0 ); + LineNumber line; if ( next_mark <= next_match ) { // LOG(logDEBUG) << "Add mark at " << next_mark; - filteredItemsCache_.push_back( FilteredItem( next_mark, Mark ) ); + type |= Mark; + line = next_mark; ++j; - if ( ( next_mark == next_match ) ) - ++i; // Case when it's both match and mark. } - else { + if ( next_mark >= next_match ) { // LOG(logDEBUG) << "Add match at " << next_match; - filteredItemsCache_.push_back( FilteredItem( next_match, Match ) ); + type |= Match; + line = next_match; ++i; } + filteredItemsCache_.push_back( FilteredItem( line, type ) ); } filteredItemsCacheDirty_ = false; diff --git a/src/data/logfiltereddata.h b/src/data/logfiltereddata.h index 4c09f9479..4aa7e9bf9 100644 --- a/src/data/logfiltereddata.h +++ b/src/data/logfiltereddata.h @@ -80,9 +80,13 @@ class LogFilteredData : public AbstractLogData { // Returns the number of marks (independently of the visibility) LineNumber getNbMarks() const; + // Flags indicating why the line is filtered. + enum FilteredLineType { + Match = 0x1, + Mark = 0x2, + }; // Returns the reason why the line at the passed index is in the filtered - // data. It can be because it is either a mark or a match. - enum FilteredLineType { Match, Mark }; + // data. FilteredLineType filteredLineTypeByIndex( int index ) const; // Marks interface (delegated to a Marks object) @@ -164,6 +168,23 @@ class LogFilteredData : public AbstractLogData { void regenerateFilteredItemsCache() const; }; +inline LogFilteredData::FilteredLineType& operator|=(LogFilteredData::FilteredLineType& a, LogFilteredData::FilteredLineType b) +{ + a = LogFilteredData::FilteredLineType( a | b ); + return a; +} + +inline LogFilteredData::FilteredLineType& operator&=(LogFilteredData::FilteredLineType& a, LogFilteredData::FilteredLineType b) +{ + a = LogFilteredData::FilteredLineType( a & b ); + return a; +} + +inline LogFilteredData::FilteredLineType operator~(LogFilteredData::FilteredLineType a) +{ + return LogFilteredData::FilteredLineType( ~static_cast( a ) ); +} + // A class representing a Mark or Match. // Conceptually it should be a base class for Mark and MatchingLine, // but we implement it this way for performance reason as we create plenty of @@ -183,6 +204,13 @@ class LogFilteredData::FilteredItem { FilteredLineType type() const { return type_; } + void add( FilteredLineType type ) + { type_ |= type; } + + // Returns whether any type-flag is left. + bool remove( FilteredLineType type ) + { return type_ &= ~type; } + bool operator <( const LogFilteredData::FilteredItem& other ) const { return lineNumber_ < other.lineNumber_; } diff --git a/src/filteredview.cpp b/src/filteredview.cpp index fa21a3332..e0f4c74f1 100644 --- a/src/filteredview.cpp +++ b/src/filteredview.cpp @@ -61,7 +61,7 @@ AbstractLogView::LineType FilteredView::lineType( int lineNumber ) const { LogFilteredData::FilteredLineType type = logFilteredData_->filteredLineTypeByIndex( lineNumber ); - if ( type == LogFilteredData::Mark ) + if ( type & LogFilteredData::Mark ) // If both Mark and Match, Mark wins return Marked; else return Match; diff --git a/src/overview.cpp b/src/overview.cpp index 17d9509a7..82aa650df 100644 --- a/src/overview.cpp +++ b/src/overview.cpp @@ -120,7 +120,7 @@ void Overview::recalculatesLines() logFilteredData_->filteredLineTypeByIndex( i ); int line = (int) logFilteredData_->getMatchingLineNumber( i ); int position = (int)( (qint64)line * height_ / linesInFile_ ); - if ( line_type == LogFilteredData::Match ) { + if ( line_type & LogFilteredData::Match ) { if ( ( ! matchLines_.isEmpty() ) && matchLines_.last().position() == position ) { // If the line is already there, we increase its weight matchLines_.last().load(); From e18d691cbd25574229e9fbba14ac073eb39814a2 Mon Sep 17 00:00:00 2001 From: Alexander Hirsch Date: Fri, 15 Jun 2018 14:21:50 +0200 Subject: [PATCH 3/6] Eagerly fill LogFilteredData::filteredItemsCache_ and progressively fill/remove items instead of completely regenerating the whole cache. --- src/data/logfiltereddata.cpp | 163 +++++++++++++++++------ src/data/logfiltereddata.h | 7 +- src/data/logfiltereddataworkerthread.cpp | 25 +++- src/data/logfiltereddataworkerthread.h | 8 +- src/log.h | 2 +- src/marks.cpp | 17 ++- src/marks.h | 10 +- 7 files changed, 176 insertions(+), 56 deletions(-) diff --git a/src/data/logfiltereddata.cpp b/src/data/logfiltereddata.cpp index 2a9335ac4..4d5de5935 100644 --- a/src/data/logfiltereddata.cpp +++ b/src/data/logfiltereddata.cpp @@ -47,8 +47,6 @@ LogFilteredData::LogFilteredData() : AbstractLogData(), maxLengthMarks_ = 0; searchDone_ = true; visibility_ = MarksAndMatches; - - filteredItemsCacheDirty_ = true; } // Usual constructor: just copy the data, the search is started by runSearch() @@ -72,8 +70,6 @@ LogFilteredData::LogFilteredData( const LogData* logData ) visibility_ = MarksAndMatches; - filteredItemsCacheDirty_ = true; - // Forward the update signal connect( &workerThread_, SIGNAL( searchProgressed( int, int, qint64 ) ), this, SLOT( handleSearchProgressed( int, int, qint64 ) ) ); @@ -123,7 +119,7 @@ void LogFilteredData::clearSearch() matching_lines_.clear(); maxLength_ = 0; nbLinesProcessed_ = 0; - filteredItemsCacheDirty_ = true; + removeAllFromFilteredItemsCache( Match ); } qint64 LogFilteredData::getMatchingLineNumber( int matchNum ) const @@ -172,11 +168,6 @@ LogFilteredData::FilteredLineType else if ( visibility_ == MarksOnly ) return Mark; else { - // If it is MarksAndMatches, we have to look. - // Regenerate the cache if needed - if ( filteredItemsCacheDirty_ ) - regenerateFilteredItemsCache(); - return filteredItemsCache_[ index ].type(); } } @@ -189,7 +180,7 @@ void LogFilteredData::addMark( qint64 line, QChar mark ) marks_.addMark( line, mark ); maxLengthMarks_ = qMax( maxLengthMarks_, sourceLogData_->getLineLength( line ) ); - filteredItemsCacheDirty_ = true; + insertIntoFilteredItemsCache( FilteredItem{ static_cast( line ), Mark } ); } else LOG(logERROR) << "LogFilteredData::addMark\ @@ -236,26 +227,41 @@ qint64 LogFilteredData::getMarkBefore( qint64 line ) const void LogFilteredData::deleteMark( QChar mark ) { - marks_.deleteMark( mark ); - filteredItemsCacheDirty_ = true; + int index = marks_.findMark( mark ); + qint64 line = marks_.getLineMarkedByIndex( index ); + marks_.deleteMarkAt( index ); - // FIXME: maxLengthMarks_ + if ( line < 0 ) { + // LOG(logWARNING)? + return; + } + + updateMaxLengthMarks( line ); + removeFromFilteredItemsCache( FilteredItem{ static_cast( line ), Mark } ); } void LogFilteredData::deleteMark( qint64 line ) { marks_.deleteMark( line ); - filteredItemsCacheDirty_ = true; + updateMaxLengthMarks( line ); + removeFromFilteredItemsCache( FilteredItem{ static_cast( line ), Mark } ); +} + +void LogFilteredData::updateMaxLengthMarks( qint64 removed_line ) +{ + if ( removed_line < 0 ) { + LOG(logWARNING) << "updateMaxLengthMarks called with negative line-number"; + return; + } // Now update the max length if needed - if ( sourceLogData_->getLineLength( line ) >= maxLengthMarks_ ) { + if ( sourceLogData_->getLineLength( removed_line ) >= maxLengthMarks_ ) { LOG(logDEBUG) << "deleteMark recalculating longest mark"; maxLengthMarks_ = 0; - for ( Marks::const_iterator i = marks_.begin(); - i != marks_.end(); ++i ) { - LOG(logDEBUG) << "line " << i->lineNumber(); + for ( auto& mark : marks_ ) { + LOG(logDEBUG) << "line " << mark.lineNumber(); maxLengthMarks_ = qMax( maxLengthMarks_, - sourceLogData_->getLineLength( i->lineNumber() ) ); + sourceLogData_->getLineLength( mark.lineNumber() ) ); } } } @@ -263,13 +269,16 @@ void LogFilteredData::deleteMark( qint64 line ) void LogFilteredData::clearMarks() { marks_.clear(); - filteredItemsCacheDirty_ = true; maxLengthMarks_ = 0; + removeAllFromFilteredItemsCache( Mark ); } void LogFilteredData::setVisibility( Visibility visi ) { visibility_ = visi; + + if ( visibility_ == MarksAndMatches ) + regenerateFilteredItemsCache(); } // @@ -277,12 +286,28 @@ void LogFilteredData::setVisibility( Visibility visi ) // void LogFilteredData::handleSearchProgressed( int nbMatches, int progress, qint64 initial_position ) { + using std::begin; + using std::end; + using std::next; + LOG(logDEBUG) << "LogFilteredData::handleSearchProgressed matches=" << nbMatches << " progress=" << progress; // searchDone_ = true; - workerThread_.getSearchResult( &maxLength_, &matching_lines_, &nbLinesProcessed_ ); - filteredItemsCacheDirty_ = true; + assert( nbMatches >= 0 ); + + size_t start_index = matching_lines_.size(); + + workerThread_.updateSearchResult( &maxLength_, &matching_lines_, &nbLinesProcessed_ ); + + assert( matching_lines_.size() >= start_index ); + + filteredItemsCache_.reserve( matching_lines_.size() + marks_.size() ); + // (it's an overestimate but probably not by much so it's fine) + + for ( auto it = next( begin( matching_lines_ ), start_index ); it != end( matching_lines_ ); ++it ) { + insertIntoFilteredItemsCache( FilteredItem{ it->lineNumber(), Match } ); + } emit searchProgressed( nbMatches, progress, initial_position ); } @@ -305,10 +330,6 @@ LineNumber LogFilteredData::findLogDataLine( LineNumber lineNum ) const LOG(logERROR) << "Index too big in LogFilteredData: " << lineNum; } else { - // Regenerate the cache if needed - if ( filteredItemsCacheDirty_ ) - regenerateFilteredItemsCache(); - if ( lineNum < filteredItemsCache_.size() ) line = filteredItemsCache_[ lineNum ].lineNumber(); else @@ -333,11 +354,6 @@ LineNumber LogFilteredData::findFilteredLine( LineNumber lineNum ) const lineNum ); } else { - // Regenerate the cache if needed - if ( filteredItemsCacheDirty_ ) { - regenerateFilteredItemsCache(); - } - lineIndex = lookupLineNumber( filteredItemsCache_.begin(), filteredItemsCache_.end(), lineNum ); @@ -398,10 +414,6 @@ qint64 LogFilteredData::doGetNbLine() const else if ( visibility_ == MarksOnly ) nbLines = marks_.size(); else { - // Regenerate the cache if needed (hopefully most of the time - // it won't be necessarily) - if ( filteredItemsCacheDirty_ ) - regenerateFilteredItemsCache(); nbLines = filteredItemsCache_.size(); } @@ -439,13 +451,16 @@ void LogFilteredData::doSetMultibyteEncodingOffsets( int, int ) { } -// TODO: We might be a bit smarter and not regenerate the whole thing when -// e.g. stuff is added at the end of the search. void LogFilteredData::regenerateFilteredItemsCache() const { LOG(logDEBUG) << "regenerateFilteredItemsCache"; - filteredItemsCache_.clear(); + if ( filteredItemsCache_.size() > 0 ) { + // the cache was not invalidated, so we can keep it + LOG(logDEBUG) << "cache was not invalidated"; + return; + } + filteredItemsCache_.reserve( matching_lines_.size() + marks_.size() ); // (it's an overestimate but probably not by much so it's fine) @@ -474,7 +489,73 @@ void LogFilteredData::regenerateFilteredItemsCache() const filteredItemsCache_.push_back( FilteredItem( line, type ) ); } - filteredItemsCacheDirty_ = false; - LOG(logDEBUG) << "finished regenerateFilteredItemsCache"; } + +void LogFilteredData::insertIntoFilteredItemsCache( FilteredItem item ) +{ + using std::begin; + using std::end; + + if ( visibility_ != MarksAndMatches ) { + // this is invalidated and will be regenerated when we need it + filteredItemsCache_.clear(); + LOG(logDEBUG) << "cache is invalidated"; + return; + } + + // Search for the corresponding index. + auto found = std::lower_bound( begin( filteredItemsCache_ ), end( filteredItemsCache_ ), item ); + if ( found == end( filteredItemsCache_ ) || found->lineNumber() > item.lineNumber() ) { + filteredItemsCache_.insert( found, item ); + } else { + assert( found->lineNumber() == item.lineNumber() ); + found->add( item.type() ); + } +} + +void LogFilteredData::removeFromFilteredItemsCache( FilteredItem item ) +{ + using std::begin; + using std::distance; + using std::end; + + if ( visibility_ != MarksAndMatches ) { + // this is invalidated and will be regenerated when we need it + filteredItemsCache_.clear(); + LOG(logDEBUG) << "cache is invalidated"; + return; + } + + // Search for the corresponding index. + auto found = std::equal_range( begin( filteredItemsCache_ ), end( filteredItemsCache_ ), item ); + if( found.first == end( filteredItemsCache_ ) ) { + LOG(logERROR) << "Attempt to remove line " << item.lineNumber() << " from filteredItemsCache_ failed, since it was not found"; + return; + } + + if ( distance( found.first, found.second ) > 1 ) { + LOG(logERROR) << "Multiple matches found for line " << item.lineNumber() << " in filteredItemsCache_"; + // FIXME: collapse them? + } + + if ( !found.first->remove( item.type() ) ){ + filteredItemsCache_.erase( found.first ); + } +} + +void LogFilteredData::removeAllFromFilteredItemsCache( FilteredLineType type ) +{ + using std::begin; + using std::end; + + if ( visibility_ != MarksAndMatches ) { + // this is invalidated and will be regenerated when we need it + filteredItemsCache_.clear(); + LOG(logDEBUG) << "cache is invalidated"; + return; + } + + auto erase_begin = std::remove_if( begin( filteredItemsCache_ ), end( filteredItemsCache_ ), [type]( FilteredItem& item ) { return !item.remove( type ); } ); + filteredItemsCache_.erase( erase_begin, end( filteredItemsCache_ ) ); +} diff --git a/src/data/logfiltereddata.h b/src/data/logfiltereddata.h index 4aa7e9bf9..48136aed5 100644 --- a/src/data/logfiltereddata.h +++ b/src/data/logfiltereddata.h @@ -156,7 +156,6 @@ class LogFilteredData : public AbstractLogData { // when visibility_ == MarksAndMatches // (QVector store actual objects instead of pointers) mutable std::vector filteredItemsCache_; - mutable bool filteredItemsCacheDirty_; LogFilteredDataWorkerThread workerThread_; Marks marks_; @@ -166,6 +165,12 @@ class LogFilteredData : public AbstractLogData { LineNumber findFilteredLine( LineNumber lineNum ) const; void regenerateFilteredItemsCache() const; + void insertIntoFilteredItemsCache( FilteredItem item ); + void removeFromFilteredItemsCache( FilteredItem item ); + void removeAllFromFilteredItemsCache( FilteredLineType type ); + + // update maxLengthMarks_ when a Mark was removed. + void updateMaxLengthMarks( qint64 removed_line ); }; inline LogFilteredData::FilteredLineType& operator|=(LogFilteredData::FilteredLineType& a, LogFilteredData::FilteredLineType b) diff --git a/src/data/logfiltereddataworkerthread.cpp b/src/data/logfiltereddataworkerthread.cpp index 76e5e1c7f..621af3578 100644 --- a/src/data/logfiltereddataworkerthread.cpp +++ b/src/data/logfiltereddataworkerthread.cpp @@ -28,15 +28,32 @@ const int SearchOperation::nbLinesInChunk = 5000; void SearchData::getAll( int* length, SearchResultArray* matches, - qint64* lines) const + qint64* lines ) const +{ + matches->clear(); + getAllMissing( length, matches, lines ); +} + +void SearchData::getAllMissing( int* length, SearchResultArray* matches, + qint64* lines ) const { QMutexLocker locker( &dataMutex_ ); + *length = maxLength_; *lines = nbLinesProcessed_; + if ( matches_.size() < matches->size() ) { + LOG(logWARNING) << "Cannot append search-data to smaller match-array"; + return; + } + + matches->reserve( matches_.size() - matches->size() ); + // This is a copy (potentially slow) - *matches = matches_; + size_t offset = matches->size(); + std::insert_iterator inserter{ *matches, next( begin( *matches ), offset ) }; + copy( next( begin( matches_ ), offset ), end( matches_ ), inserter ); } void SearchData::setAll( int length, @@ -167,10 +184,10 @@ void LogFilteredDataWorkerThread::interrupt() } // This will do an atomic copy of the object -void LogFilteredDataWorkerThread::getSearchResult( +void LogFilteredDataWorkerThread::updateSearchResult( int* maxLength, SearchResultArray* searchMatches, qint64* nbLinesProcessed ) { - searchData_.getAll( maxLength, searchMatches, nbLinesProcessed ); + searchData_.getAllMissing( maxLength, searchMatches, nbLinesProcessed ); } // This is the thread's main loop diff --git a/src/data/logfiltereddataworkerthread.h b/src/data/logfiltereddataworkerthread.h index 5b8f54553..6ab85460b 100644 --- a/src/data/logfiltereddataworkerthread.h +++ b/src/data/logfiltereddataworkerthread.h @@ -63,6 +63,10 @@ class SearchData // Atomically get all the search data void getAll( int* length, SearchResultArray* matches, qint64* nbLinesProcessed ) const; + // Atomically get all the search data + // Appends the missing entries. Does not check that the existing entries match. + void getAllMissing( int* length, SearchResultArray* matches, + qint64* lines ) const; // Atomically set all the search data // (overwriting the existing) // (the matches are always moved) @@ -155,8 +159,8 @@ class LogFilteredDataWorkerThread : public QThread // Interrupts the search if one is in progress void interrupt(); - // Returns a copy of the current indexing data - void getSearchResult( int* maxLength, SearchResultArray* searchMatches, + // Updates the array by copying the current indexing data + void updateSearchResult( int* maxLength, SearchResultArray* searchMatches, qint64* nbLinesProcessed ); signals: diff --git a/src/log.h b/src/log.h index 827c8f8a7..893367b34 100644 --- a/src/log.h +++ b/src/log.h @@ -25,7 +25,7 @@ #include // Modify here! -//#define FILELOG_MAX_LEVEL logDEBUG +// #define FILELOG_MAX_LEVEL logDEBUG inline std::string NowTime(); diff --git a/src/marks.cpp b/src/marks.cpp index 0abcf4115..9201331c7 100644 --- a/src/marks.cpp +++ b/src/marks.cpp @@ -66,20 +66,29 @@ bool Marks::isLineMarked( qint64 line ) const return lookupLineNumber< QList >( marks_, line, &index ); } -void Marks::deleteMark( QChar mark ) +int Marks::findMark( QChar mark ) { // 'mark' is not used yet mark = mark; + + return -1; } -void Marks::deleteMark( qint64 line ) +void Marks::deleteMarkAt( int index ) { - int index; + marks_.removeAt( index ); +} + +int Marks::deleteMark( qint64 line ) +{ + int index = -1; if ( lookupLineNumber< QList >( marks_, line, &index ) ) { - marks_.removeAt( index ); + deleteMarkAt( index ); } + + return index; } void Marks::clear() diff --git a/src/marks.h b/src/marks.h index 6871d3d57..e55ac6376 100644 --- a/src/marks.h +++ b/src/marks.h @@ -54,11 +54,15 @@ class Marks { qint64 getMark( QChar mark ) const; // Returns wheither the passed line has a mark on it. bool isLineMarked( qint64 line ) const; - // Delete the mark identified by the passed char. - void deleteMark( QChar mark ); + // Find a mark. + // Returns the index of the mark or -1 if it was not found. + int findMark( QChar mark ); + // Delete the mark at an index. + void deleteMarkAt( int index ); // Delete the mark present on the passed line or do nothing if there is // none. - void deleteMark( qint64 line ); + // Returns the index where the mark used to be. + int deleteMark( qint64 line ); // Get the line marked identified by the index (in this list) passed. qint64 getLineMarkedByIndex( int index ) const { return marks_[index].lineNumber(); } From 70538f421e4c53f2fd46af5484c5aef09f7f183b Mon Sep 17 00:00:00 2001 From: Alexander Hirsch Date: Fri, 15 Jun 2018 14:24:40 +0200 Subject: [PATCH 4/6] Optimize filteredItemsCache_ filling by passing in a guess for the index. --- src/data/logfiltereddata.cpp | 72 +++++++++++++++++++++++++++--------- src/data/logfiltereddata.h | 8 ++++ src/marks.cpp | 4 +- src/marks.h | 4 +- 4 files changed, 68 insertions(+), 20 deletions(-) diff --git a/src/data/logfiltereddata.cpp b/src/data/logfiltereddata.cpp index 4d5de5935..cc231ef15 100644 --- a/src/data/logfiltereddata.cpp +++ b/src/data/logfiltereddata.cpp @@ -177,10 +177,10 @@ LogFilteredData::FilteredLineType void LogFilteredData::addMark( qint64 line, QChar mark ) { if ( ( line >= 0 ) && ( line < sourceLogData_->getNbLine() ) ) { - marks_.addMark( line, mark ); + int index = marks_.addMark( line, mark ); maxLengthMarks_ = qMax( maxLengthMarks_, sourceLogData_->getLineLength( line ) ); - insertIntoFilteredItemsCache( FilteredItem{ static_cast( line ), Mark } ); + insertIntoFilteredItemsCache( index, FilteredItem{ static_cast( line ), Mark } ); } else LOG(logERROR) << "LogFilteredData::addMark\ @@ -237,15 +237,15 @@ void LogFilteredData::deleteMark( QChar mark ) } updateMaxLengthMarks( line ); - removeFromFilteredItemsCache( FilteredItem{ static_cast( line ), Mark } ); + removeFromFilteredItemsCache( index, FilteredItem{ static_cast( line ), Mark } ); } void LogFilteredData::deleteMark( qint64 line ) { - marks_.deleteMark( line ); + int index = marks_.deleteMark( line ); updateMaxLengthMarks( line ); - removeFromFilteredItemsCache( FilteredItem{ static_cast( line ), Mark } ); + removeFromFilteredItemsCache( index, FilteredItem{ static_cast( line ), Mark } ); } void LogFilteredData::updateMaxLengthMarks( qint64 removed_line ) @@ -300,14 +300,7 @@ void LogFilteredData::handleSearchProgressed( int nbMatches, int progress, qint6 workerThread_.updateSearchResult( &maxLength_, &matching_lines_, &nbLinesProcessed_ ); - assert( matching_lines_.size() >= start_index ); - - filteredItemsCache_.reserve( matching_lines_.size() + marks_.size() ); - // (it's an overestimate but probably not by much so it's fine) - - for ( auto it = next( begin( matching_lines_ ), start_index ); it != end( matching_lines_ ); ++it ) { - insertIntoFilteredItemsCache( FilteredItem{ it->lineNumber(), Match } ); - } + insertMatchesIntoFilteredItemsCache( start_index ); emit searchProgressed( nbMatches, progress, initial_position ); } @@ -492,20 +485,21 @@ void LogFilteredData::regenerateFilteredItemsCache() const LOG(logDEBUG) << "finished regenerateFilteredItemsCache"; } -void LogFilteredData::insertIntoFilteredItemsCache( FilteredItem item ) +void LogFilteredData::insertIntoFilteredItemsCache( size_t insert_index, FilteredItem item ) { using std::begin; using std::end; + using std::next; if ( visibility_ != MarksAndMatches ) { // this is invalidated and will be regenerated when we need it filteredItemsCache_.clear(); LOG(logDEBUG) << "cache is invalidated"; - return; } // Search for the corresponding index. - auto found = std::lower_bound( begin( filteredItemsCache_ ), end( filteredItemsCache_ ), item ); + // We can start the search from insert_index, since lineNumber >= index is always true. + auto found = std::lower_bound( next( begin( filteredItemsCache_ ), insert_index ), end( filteredItemsCache_ ), item ); if ( found == end( filteredItemsCache_ ) || found->lineNumber() > item.lineNumber() ) { filteredItemsCache_.insert( found, item ); } else { @@ -514,11 +508,52 @@ void LogFilteredData::insertIntoFilteredItemsCache( FilteredItem item ) } } -void LogFilteredData::removeFromFilteredItemsCache( FilteredItem item ) +void LogFilteredData::insertIntoFilteredItemsCache( FilteredItem item ) +{ + insertIntoFilteredItemsCache( 0, item ); +} + +void LogFilteredData::insertMatchesIntoFilteredItemsCache( size_t start_index ) +{ + using std::begin; + using std::end; + using std::next; + + assert( start_index <= matching_lines_.size() ); + + if ( visibility_ != MarksAndMatches ) { + // this is invalidated and will be regenerated when we need it + filteredItemsCache_.clear(); + LOG(logDEBUG) << "cache is invalidated"; + return; + } + + assert( start_index <= filteredItemsCache_.size() ); + + filteredItemsCache_.reserve( matching_lines_.size() + marks_.size() ); + // (it's an overestimate but probably not by much so it's fine) + + // Search for the corresponding index. + // We can start the search from insert_index, since lineNumber >= index is always true. + auto filteredIt = next( begin( filteredItemsCache_ ), start_index ); + for ( auto matchesIt = next( begin( matching_lines_ ), start_index ); matchesIt != end( matching_lines_ ); ++matchesIt ) { + FilteredItem item{ matchesIt->lineNumber(), Match }; + filteredIt = std::lower_bound( filteredIt, end( filteredItemsCache_ ), item ); + if ( filteredIt == end( filteredItemsCache_ ) || filteredIt->lineNumber() > item.lineNumber() ) { + filteredIt = filteredItemsCache_.insert( filteredIt, item ); + } else { + assert( filteredIt->lineNumber() == matchesIt->lineNumber() ); + filteredIt->add( item.type() ); + } + } +} + +void LogFilteredData::removeFromFilteredItemsCache( size_t remove_index, FilteredItem item ) { using std::begin; using std::distance; using std::end; + using std::next; if ( visibility_ != MarksAndMatches ) { // this is invalidated and will be regenerated when we need it @@ -528,7 +563,8 @@ void LogFilteredData::removeFromFilteredItemsCache( FilteredItem item ) } // Search for the corresponding index. - auto found = std::equal_range( begin( filteredItemsCache_ ), end( filteredItemsCache_ ), item ); + // We can start the search from remove_index, since lineNumber >= index is always true. + auto found = std::equal_range( next( begin( filteredItemsCache_ ), remove_index ), end( filteredItemsCache_ ), item ); if( found.first == end( filteredItemsCache_ ) ) { LOG(logERROR) << "Attempt to remove line " << item.lineNumber() << " from filteredItemsCache_ failed, since it was not found"; return; diff --git a/src/data/logfiltereddata.h b/src/data/logfiltereddata.h index 48136aed5..7eca098f4 100644 --- a/src/data/logfiltereddata.h +++ b/src/data/logfiltereddata.h @@ -165,7 +165,15 @@ class LogFilteredData : public AbstractLogData { LineNumber findFilteredLine( LineNumber lineNum ) const; void regenerateFilteredItemsCache() const; + // start_index can be passed in as an optimization when finding the item. + // It refers to the index of the singular arrays (Marks or SearchResultArray) where the item was inserted. + void insertIntoFilteredItemsCache( size_t start_index, FilteredItem item ); void insertIntoFilteredItemsCache( FilteredItem item ); + // Insert entries from matching_lines_ into filteredItemsCache_ starting by start_index. + void insertMatchesIntoFilteredItemsCache( size_t start_index ); + // remove_index can be passed in as an optimization when finding the item. + // It refers to the index of the singular arrays (Marks or SearchResultArray) where the item was removed. + void removeFromFilteredItemsCache( size_t remove_index, FilteredItem item ); void removeFromFilteredItemsCache( FilteredItem item ); void removeAllFromFilteredItemsCache( FilteredLineType type ); diff --git a/src/marks.cpp b/src/marks.cpp index 9201331c7..d3f4d2036 100644 --- a/src/marks.cpp +++ b/src/marks.cpp @@ -32,7 +32,7 @@ Marks::Marks() : marks_() { } -void Marks::addMark( qint64 line, QChar mark ) +int Marks::addMark( qint64 line, QChar mark ) { // Look for the index immediately before int index; @@ -50,6 +50,8 @@ void Marks::addMark( qint64 line, QChar mark ) // 'mark' is not used yet mark = mark; + + return index; } qint64 Marks::getMark( QChar mark ) const diff --git a/src/marks.h b/src/marks.h index e55ac6376..3a935ff42 100644 --- a/src/marks.h +++ b/src/marks.h @@ -49,7 +49,9 @@ class Marks { // Add a mark at the given line, optionally identified by the given char // If a mark for this char already exist, the previous one is replaced. // It will happily add marks anywhere, even at stupid indexes. - void addMark( qint64 line, QChar mark = QChar() ); + // Returns the index at which the mark was inserted, + // such that getLineMarkedByIndex( addMark( line ) ) == line. + int addMark( qint64 line, QChar mark = QChar() ); // Get the (unique) mark identified by the passed char. qint64 getMark( QChar mark ) const; // Returns wheither the passed line has a mark on it. From 0e73a3db81446698db6047828aefa1ee553619a9 Mon Sep 17 00:00:00 2001 From: Alexander Hirsch Date: Fri, 1 Mar 2019 11:27:49 +0100 Subject: [PATCH 5/6] Refactor src/data/logfiltereddata.cpp --- src/data/logfiltereddata.cpp | 42 ++++++++++++++++++++---------------- src/data/logfiltereddata.h | 15 ++++++------- 2 files changed, 29 insertions(+), 28 deletions(-) diff --git a/src/data/logfiltereddata.cpp b/src/data/logfiltereddata.cpp index cc231ef15..f32c01390 100644 --- a/src/data/logfiltereddata.cpp +++ b/src/data/logfiltereddata.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include "utils.h" #include "logdata.h" @@ -180,7 +181,7 @@ void LogFilteredData::addMark( qint64 line, QChar mark ) int index = marks_.addMark( line, mark ); maxLengthMarks_ = qMax( maxLengthMarks_, sourceLogData_->getLineLength( line ) ); - insertIntoFilteredItemsCache( index, FilteredItem{ static_cast( line ), Mark } ); + insertIntoFilteredItemsCache( index, { static_cast( line ), Mark } ); } else LOG(logERROR) << "LogFilteredData::addMark\ @@ -237,7 +238,7 @@ void LogFilteredData::deleteMark( QChar mark ) } updateMaxLengthMarks( line ); - removeFromFilteredItemsCache( index, FilteredItem{ static_cast( line ), Mark } ); + removeFromFilteredItemsCache( index, { static_cast( line ), Mark } ); } void LogFilteredData::deleteMark( qint64 line ) @@ -245,7 +246,7 @@ void LogFilteredData::deleteMark( qint64 line ) int index = marks_.deleteMark( line ); updateMaxLengthMarks( line ); - removeFromFilteredItemsCache( index, FilteredItem{ static_cast( line ), Mark } ); + removeFromFilteredItemsCache( index, { static_cast( line ), Mark } ); } void LogFilteredData::updateMaxLengthMarks( qint64 removed_line ) @@ -479,13 +480,13 @@ void LogFilteredData::regenerateFilteredItemsCache() const line = next_match; ++i; } - filteredItemsCache_.push_back( FilteredItem( line, type ) ); + filteredItemsCache_.emplace_back( line, type ); } LOG(logDEBUG) << "finished regenerateFilteredItemsCache"; } -void LogFilteredData::insertIntoFilteredItemsCache( size_t insert_index, FilteredItem item ) +void LogFilteredData::insertIntoFilteredItemsCache( size_t insert_index, FilteredItem &&item ) { using std::begin; using std::end; @@ -501,16 +502,16 @@ void LogFilteredData::insertIntoFilteredItemsCache( size_t insert_index, Filtere // We can start the search from insert_index, since lineNumber >= index is always true. auto found = std::lower_bound( next( begin( filteredItemsCache_ ), insert_index ), end( filteredItemsCache_ ), item ); if ( found == end( filteredItemsCache_ ) || found->lineNumber() > item.lineNumber() ) { - filteredItemsCache_.insert( found, item ); + filteredItemsCache_.emplace( found, std::move( item ) ); } else { - assert( found->lineNumber() == item.lineNumber() ); + Q_ASSERT( found->lineNumber() == item.lineNumber() ); found->add( item.type() ); } } -void LogFilteredData::insertIntoFilteredItemsCache( FilteredItem item ) +void LogFilteredData::insertIntoFilteredItemsCache( FilteredItem &&item ) { - insertIntoFilteredItemsCache( 0, item ); + return insertIntoFilteredItemsCache( 0, std::move( item ) ); } void LogFilteredData::insertMatchesIntoFilteredItemsCache( size_t start_index ) @@ -537,21 +538,19 @@ void LogFilteredData::insertMatchesIntoFilteredItemsCache( size_t start_index ) // We can start the search from insert_index, since lineNumber >= index is always true. auto filteredIt = next( begin( filteredItemsCache_ ), start_index ); for ( auto matchesIt = next( begin( matching_lines_ ), start_index ); matchesIt != end( matching_lines_ ); ++matchesIt ) { - FilteredItem item{ matchesIt->lineNumber(), Match }; - filteredIt = std::lower_bound( filteredIt, end( filteredItemsCache_ ), item ); - if ( filteredIt == end( filteredItemsCache_ ) || filteredIt->lineNumber() > item.lineNumber() ) { - filteredIt = filteredItemsCache_.insert( filteredIt, item ); + filteredIt = std::lower_bound( filteredIt, end( filteredItemsCache_ ), matchesIt->lineNumber() ); + if ( filteredIt == end( filteredItemsCache_ ) || filteredIt->lineNumber() > matchesIt->lineNumber() ) { + filteredIt = filteredItemsCache_.emplace( filteredIt, matchesIt->lineNumber(), Match ); } else { assert( filteredIt->lineNumber() == matchesIt->lineNumber() ); - filteredIt->add( item.type() ); + filteredIt->add( Match ); } } } -void LogFilteredData::removeFromFilteredItemsCache( size_t remove_index, FilteredItem item ) +void LogFilteredData::removeFromFilteredItemsCache( size_t remove_index, const FilteredItem &item ) { using std::begin; - using std::distance; using std::end; using std::next; @@ -565,21 +564,26 @@ void LogFilteredData::removeFromFilteredItemsCache( size_t remove_index, Filtere // Search for the corresponding index. // We can start the search from remove_index, since lineNumber >= index is always true. auto found = std::equal_range( next( begin( filteredItemsCache_ ), remove_index ), end( filteredItemsCache_ ), item ); - if( found.first == end( filteredItemsCache_ ) ) { + if( found.first == found.second ) { LOG(logERROR) << "Attempt to remove line " << item.lineNumber() << " from filteredItemsCache_ failed, since it was not found"; return; } - if ( distance( found.first, found.second ) > 1 ) { + if ( next( found.first ) != found.second ) { LOG(logERROR) << "Multiple matches found for line " << item.lineNumber() << " in filteredItemsCache_"; // FIXME: collapse them? } - if ( !found.first->remove( item.type() ) ){ + if ( !found.first->remove( item.type() ) ) { filteredItemsCache_.erase( found.first ); } } +void LogFilteredData::removeFromFilteredItemsCache( const FilteredItem &item ) +{ + removeFromFilteredItemsCache( 0, item ); +} + void LogFilteredData::removeAllFromFilteredItemsCache( FilteredLineType type ) { using std::begin; diff --git a/src/data/logfiltereddata.h b/src/data/logfiltereddata.h index 7eca098f4..f0703b283 100644 --- a/src/data/logfiltereddata.h +++ b/src/data/logfiltereddata.h @@ -167,14 +167,14 @@ class LogFilteredData : public AbstractLogData { void regenerateFilteredItemsCache() const; // start_index can be passed in as an optimization when finding the item. // It refers to the index of the singular arrays (Marks or SearchResultArray) where the item was inserted. - void insertIntoFilteredItemsCache( size_t start_index, FilteredItem item ); - void insertIntoFilteredItemsCache( FilteredItem item ); + void insertIntoFilteredItemsCache( size_t start_index, FilteredItem &&item ); + void insertIntoFilteredItemsCache( FilteredItem &&item ); // Insert entries from matching_lines_ into filteredItemsCache_ starting by start_index. void insertMatchesIntoFilteredItemsCache( size_t start_index ); // remove_index can be passed in as an optimization when finding the item. // It refers to the index of the singular arrays (Marks or SearchResultArray) where the item was removed. - void removeFromFilteredItemsCache( size_t remove_index, FilteredItem item ); - void removeFromFilteredItemsCache( FilteredItem item ); + void removeFromFilteredItemsCache( size_t remove_index, const FilteredItem &item ); + void removeFromFilteredItemsCache( const FilteredItem &item ); void removeAllFromFilteredItemsCache( FilteredLineType type ); // update maxLengthMarks_ when a Mark was removed. @@ -206,11 +206,8 @@ inline LogFilteredData::FilteredLineType operator~(LogFilteredData::FilteredLine // of pointer (less small allocations and no RTTI). class LogFilteredData::FilteredItem { public: - // A default ctor seems to be necessary for QVector - FilteredItem() - { lineNumber_ = 0; } FilteredItem( LineNumber lineNumber, FilteredLineType type ) - { lineNumber_ = lineNumber; type_ = type; } + : lineNumber_{ lineNumber }, type_{ type } {} LineNumber lineNumber() const { return lineNumber_; } @@ -225,7 +222,7 @@ class LogFilteredData::FilteredItem { { return type_ &= ~type; } bool operator <( const LogFilteredData::FilteredItem& other ) const - { return lineNumber_ < other.lineNumber_; } + { return *this < other.lineNumber_; } bool operator <( const LineNumber& lineNumber ) const { return lineNumber_ < lineNumber; } From cc302f3112d138eaf6e28ae96915d536a555c950 Mon Sep 17 00:00:00 2001 From: Alexander Hirsch Date: Fri, 1 Mar 2019 11:33:23 +0100 Subject: [PATCH 6/6] Favor Q_ASSERT() to assert() --- src/data/logfiltereddata.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/data/logfiltereddata.cpp b/src/data/logfiltereddata.cpp index f32c01390..b79cbea36 100644 --- a/src/data/logfiltereddata.cpp +++ b/src/data/logfiltereddata.cpp @@ -23,8 +23,9 @@ #include "log.h" +#include #include -#include +#include #include #include @@ -295,7 +296,7 @@ void LogFilteredData::handleSearchProgressed( int nbMatches, int progress, qint6 << nbMatches << " progress=" << progress; // searchDone_ = true; - assert( nbMatches >= 0 ); + Q_ASSERT( nbMatches >= 0 ); size_t start_index = matching_lines_.size(); @@ -520,7 +521,7 @@ void LogFilteredData::insertMatchesIntoFilteredItemsCache( size_t start_index ) using std::end; using std::next; - assert( start_index <= matching_lines_.size() ); + Q_ASSERT( start_index <= matching_lines_.size() ); if ( visibility_ != MarksAndMatches ) { // this is invalidated and will be regenerated when we need it @@ -529,7 +530,7 @@ void LogFilteredData::insertMatchesIntoFilteredItemsCache( size_t start_index ) return; } - assert( start_index <= filteredItemsCache_.size() ); + Q_ASSERT( start_index <= filteredItemsCache_.size() ); filteredItemsCache_.reserve( matching_lines_.size() + marks_.size() ); // (it's an overestimate but probably not by much so it's fine) @@ -542,7 +543,7 @@ void LogFilteredData::insertMatchesIntoFilteredItemsCache( size_t start_index ) if ( filteredIt == end( filteredItemsCache_ ) || filteredIt->lineNumber() > matchesIt->lineNumber() ) { filteredIt = filteredItemsCache_.emplace( filteredIt, matchesIt->lineNumber(), Match ); } else { - assert( filteredIt->lineNumber() == matchesIt->lineNumber() ); + Q_ASSERT( filteredIt->lineNumber() == matchesIt->lineNumber() ); filteredIt->add( Match ); } }