From a61c03a6a45c71c7a299f4dec3e00fc8282695e1 Mon Sep 17 00:00:00 2001 From: Mark Olesen Date: Sat, 31 Oct 2009 00:03:02 +0100 Subject: [PATCH] HashTbl: improve efficiency when deleting many keys and in operator== - fix off by one error in erase/increment code --- .../containers/HashTables/HashTbl/HashTbl.C | 56 ++++++++++--------- .../containers/HashTables/HashTbl/HashTbl.H | 5 +- .../containers/HashTables/HashTbl/HashTblI.H | 6 +- 3 files changed, 35 insertions(+), 32 deletions(-) diff --git a/src/OpenFOAM/containers/HashTables/HashTbl/HashTbl.C b/src/OpenFOAM/containers/HashTables/HashTbl/HashTbl.C index 2b9df7f349..401638e914 100644 --- a/src/OpenFOAM/containers/HashTables/HashTbl/HashTbl.C +++ b/src/OpenFOAM/containers/HashTables/HashTbl/HashTbl.C @@ -341,10 +341,15 @@ bool Foam::HashTbl::set } +// NOTE: +// We use (const iterator&) here, but manipulate its contents anyhow. +// The parameter should be (iterator&), but then the compiler doesn't find +// it correctly and tries to call as (iterator) instead. +// template bool Foam::HashTbl::erase(const iterator& cit) { - // note: endIter_ has NULL elmtPtr_, so this also catches that + // note: elmtPtr_ is NULL for end(), so this catches that too if (cit.elmtPtr_) { // Search element before elmtPtr_ @@ -378,8 +383,14 @@ bool Foam::HashTbl::erase(const iterator& cit) // assign an non-NULL value so it doesn't look like end()/cend() iter.elmtPtr_ = reinterpret_cast(this); - // mark with special hashIndex value to signal it has been rewound - // the next increment will bring it back to the present location + // Mark with special hashIndex value to signal it has been rewound. + // The next increment will bring it back to the present location. + // + // For the current position 'X', mark it as '-(X+1)', which is + // written as '-X-1' to avoid overflow. + // The extra '-1' is needed to avoid ambiguity for position '0'. + // To retrieve the previous position 'X-1' we would later + // use '-(-X-1) - 2' iter.hashIndex_ = -iter.hashIndex_ - 1; } @@ -413,6 +424,7 @@ bool Foam::HashTbl::erase(const iterator& cit) template bool Foam::HashTbl::erase(const Key& key) { + // we could also simply do: erase(find(key)); iterator fnd = find(key); if (fnd != end()) @@ -429,17 +441,15 @@ bool Foam::HashTbl::erase(const Key& key) template Foam::label Foam::HashTbl::erase(const UList& keys) { + const label nTotal = nElmts_; label count = 0; - // Remove listed keys from this table - if (this->size()) + // Remove listed keys from this table - terminates early if possible + for (label keyI = 0; count < nTotal && keyI < keys.size(); ++keyI) { - forAll(keys, keyI) + if (erase(keys[keyI])) { - if (erase(keys[keyI])) - { - count++; - } + count++; } } @@ -456,16 +466,13 @@ Foam::label Foam::HashTbl::erase { label count = 0; - // Remove rhs elements from this table - if (this->size()) + // Remove rhs keys from this table - terminates early if possible + // Could optimize depending on which hash is smaller ... + for (iterator iter = begin(); iter != end(); ++iter) { - // NOTE: could further optimize depending on which hash is smaller - for (iterator iter = begin(); iter != end(); ++iter) + if (rhs.found(iter.key()) && erase(iter)) { - if (rhs.found(iter.key()) && erase(iter)) - { - count++; - } + count++; } } @@ -618,18 +625,12 @@ bool Foam::HashTbl::operator== const HashTbl& rhs ) const { - // Are all my elements in rhs? - for (const_iterator iter = cbegin(); iter != cend(); ++iter) + // sizes (ie, number of keys) must match + if (size() != rhs.size()) { - const_iterator fnd = rhs.find(iter.key()); - - if (fnd == rhs.cend() || fnd() != iter()) - { - return false; - } + return false; } - // Are all rhs elements in me? for (const_iterator iter = rhs.cbegin(); iter != rhs.cend(); ++iter) { const_iterator fnd = find(iter.key()); @@ -639,6 +640,7 @@ bool Foam::HashTbl::operator== return false; } } + return true; } diff --git a/src/OpenFOAM/containers/HashTables/HashTbl/HashTbl.H b/src/OpenFOAM/containers/HashTables/HashTbl/HashTbl.H index d86c7a7c47..8ccd31cf14 100644 --- a/src/OpenFOAM/containers/HashTables/HashTbl/HashTbl.H +++ b/src/OpenFOAM/containers/HashTables/HashTbl/HashTbl.H @@ -269,9 +269,8 @@ public: //- Assignment void operator=(const HashTbl&); - //- Equality. Two hash tables are equal if all contents of first are - // also in second and vice versa. So does not depend on table size or - // order! + //- Equality. Hash tables are equal if the keys and values are equal. + // Independent of table storage size and table order. bool operator==(const HashTbl&) const; //- The opposite of the equality operation. Takes linear time. diff --git a/src/OpenFOAM/containers/HashTables/HashTbl/HashTblI.H b/src/OpenFOAM/containers/HashTables/HashTbl/HashTblI.H index 4af5fa3f8b..1f87b60621 100644 --- a/src/OpenFOAM/containers/HashTables/HashTbl/HashTblI.H +++ b/src/OpenFOAM/containers/HashTables/HashTbl/HashTblI.H @@ -261,10 +261,12 @@ inline typename Foam::HashTbl::iterator& Foam::HashTbl::iterator::operator++() { - // Check for special value (from erase) - this indicates the previous bin + // A negative index is a special value from erase if (hashIndex_ < 0) { - hashIndex_ = -hashIndex_; + // old position 'X' was marked as '-(X+1)' + // but we wish to continue at 'X-1' + hashIndex_ = -hashIndex_ - 2; } else if (elmtPtr_ && elmtPtr_->next_) {