From 4c6b5f0a8b2a53dd5d315a5fa4cc39fb84fce255 Mon Sep 17 00:00:00 2001 From: hogliux Date: Thu, 29 Jun 2017 17:25:38 +0100 Subject: [PATCH] Cleaned up some code in HashMap and added a getReference method --- modules/juce_core/containers/juce_HashMap.h | 116 +++++--- .../containers/juce_HashMap_test.cpp | 271 ++++++++++++++++++ modules/juce_core/juce_core.cpp | 5 + 3 files changed, 346 insertions(+), 46 deletions(-) create mode 100644 modules/juce_core/containers/juce_HashMap_test.cpp diff --git a/modules/juce_core/containers/juce_HashMap.h b/modules/juce_core/containers/juce_HashMap.h index 350944a192..e44a182493 100644 --- a/modules/juce_core/containers/juce_HashMap.h +++ b/modules/juce_core/containers/juce_HashMap.h @@ -130,9 +130,9 @@ public: { const ScopedLockType sl (getLock()); - for (int i = hashSlots.size(); --i >= 0;) + for (auto i = hashSlots.size(); --i >= 0;) { - HashEntry* h = hashSlots.getUnchecked(i); + auto* h = hashSlots.getUnchecked(i); while (h != nullptr) { @@ -161,24 +161,44 @@ public: { const ScopedLockType sl (getLock()); - for (const HashEntry* entry = hashSlots.getUnchecked (generateHashFor (keyToLookFor)); entry != nullptr; entry = entry->nextEntry) - if (entry->key == keyToLookFor) - return entry->value; + if (auto* entry = getEntry (getSlot (keyToLookFor), keyToLookFor)) + return entry->value; return ValueType(); } + /** Returns a reference to the value corresponding to a given key. + If the map doesn't contain the key, a default instance of the value type is + added to the map and a reference to this is returned. + @param keyToLookFor the key of the item being requested + */ + inline ValueType& getReference (KeyTypeParameter keyToLookFor) + { + const ScopedLockType sl (getLock()); + auto hashIndex = generateHashFor (keyToLookFor, getNumSlots()); + + auto* firstEntry = hashSlots.getUnchecked (hashIndex); + + if (auto* entry = getEntry (firstEntry, keyToLookFor)) + return entry->value; + + auto* entry = new HashEntry (keyToLookFor, ValueType(), firstEntry); + hashSlots.set (hashIndex, entry); + ++totalNumItems; + + if (totalNumItems > (getNumSlots() * 3) / 2) + remapTable (getNumSlots() * 2); + + return entry->value; + } + //============================================================================== /** Returns true if the map contains an item with the specied key. */ bool contains (KeyTypeParameter keyToLookFor) const { const ScopedLockType sl (getLock()); - for (const HashEntry* entry = hashSlots.getUnchecked (generateHashFor (keyToLookFor)); entry != nullptr; entry = entry->nextEntry) - if (entry->key == keyToLookFor) - return true; - - return false; + return (getEntry (getSlot (keyToLookFor), keyToLookFor) != nullptr); } /** Returns true if the hash contains at least one occurrence of a given value. */ @@ -186,8 +206,8 @@ public: { const ScopedLockType sl (getLock()); - for (int i = getNumSlots(); --i >= 0;) - for (const HashEntry* entry = hashSlots.getUnchecked(i); entry != nullptr; entry = entry->nextEntry) + for (auto i = getNumSlots(); --i >= 0;) + for (auto* entry = hashSlots.getUnchecked(i); entry != nullptr; entry = entry->nextEntry) if (entry->value == valueToLookFor) return true; @@ -199,35 +219,14 @@ public: If there's already an item with the given key, this will replace its value. Otherwise, a new item will be added to the map. */ - void set (KeyTypeParameter newKey, ValueTypeParameter newValue) - { - const ScopedLockType sl (getLock()); - const int hashIndex = generateHashFor (newKey); - - HashEntry* const firstEntry = hashSlots.getUnchecked (hashIndex); - - for (HashEntry* entry = firstEntry; entry != nullptr; entry = entry->nextEntry) - { - if (entry->key == newKey) - { - entry->value = newValue; - return; - } - } - - hashSlots.set (hashIndex, new HashEntry (newKey, newValue, firstEntry)); - ++totalNumItems; - - if (totalNumItems > (getNumSlots() * 3) / 2) - remapTable (getNumSlots() * 2); - } + void set (KeyTypeParameter newKey, ValueTypeParameter newValue) { getReference (newKey) = newValue; } /** Removes an item with the given key. */ void remove (KeyTypeParameter keyToRemove) { const ScopedLockType sl (getLock()); - const int hashIndex = generateHashFor (keyToRemove); - HashEntry* entry = hashSlots.getUnchecked (hashIndex); + auto hashIndex = generateHashFor (keyToRemove, getNumSlots()); + auto* entry = hashSlots.getUnchecked (hashIndex); HashEntry* previous = nullptr; while (entry != nullptr) @@ -258,9 +257,9 @@ public: { const ScopedLockType sl (getLock()); - for (int i = getNumSlots(); --i >= 0;) + for (auto i = getNumSlots(); --i >= 0;) { - HashEntry* entry = hashSlots.getUnchecked(i); + auto* entry = hashSlots.getUnchecked(i); HashEntry* previous = nullptr; while (entry != nullptr) @@ -293,13 +292,27 @@ public: */ void remapTable (int newNumberOfSlots) { - HashMap newTable (newNumberOfSlots); + const ScopedLockType sl (getLock()); - for (int i = getNumSlots(); --i >= 0;) - for (const HashEntry* entry = hashSlots.getUnchecked(i); entry != nullptr; entry = entry->nextEntry) - newTable.set (entry->key, entry->value); + Array newSlots; + newSlots.insertMultiple (0, nullptr, newNumberOfSlots); - swapWith (newTable); + for (auto i = getNumSlots(); --i >= 0;) + { + HashEntry* nextEntry = nullptr; + + for (auto* entry = hashSlots.getUnchecked(i); entry != nullptr; entry = nextEntry) + { + auto hashIndex = generateHashFor (entry->key, newNumberOfSlots); + + nextEntry = entry->nextEntry; + entry->nextEntry = newSlots.getUnchecked (hashIndex); + + newSlots.set (hashIndex, entry); + } + } + + hashSlots.swapWith (newSlots); } /** Returns the number of slots which are available for hashing. @@ -459,12 +472,23 @@ private: int totalNumItems; TypeOfCriticalSectionToUse lock; - int generateHashFor (KeyTypeParameter key) const + int generateHashFor (KeyTypeParameter key, int numSlots) const { - const int hash = hashFunctionToUse.generateHash (key, getNumSlots()); - jassert (isPositiveAndBelow (hash, getNumSlots())); // your hash function is generating out-of-range numbers! + const int hash = hashFunctionToUse.generateHash (key, numSlots); + jassert (isPositiveAndBelow (hash, numSlots)); // your hash function is generating out-of-range numbers! return hash; } + static inline HashEntry* getEntry (HashEntry* firstEntry, KeyType keyToLookFor) noexcept + { + for (auto* entry = firstEntry; entry != nullptr; entry = entry->nextEntry) + if (entry->key == keyToLookFor) + return entry; + + return nullptr; + } + + inline HashEntry* getSlot (KeyType key) const noexcept { return hashSlots.getUnchecked (generateHashFor (key, getNumSlots())); } + JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (HashMap) }; diff --git a/modules/juce_core/containers/juce_HashMap_test.cpp b/modules/juce_core/containers/juce_HashMap_test.cpp new file mode 100644 index 0000000000..45c006a318 --- /dev/null +++ b/modules/juce_core/containers/juce_HashMap_test.cpp @@ -0,0 +1,271 @@ +/* + ============================================================================== + + This file is part of the JUCE library. + Copyright (c) 2017 - ROLI Ltd. + + JUCE is an open source library subject to commercial or open-source + licensing. + + The code included in this file is provided under the terms of the ISC license + http://www.isc.org/downloads/software-support-policy/isc-license. Permission + To use, copy, modify, and/or distribute this software for any purpose with or + without fee is hereby granted provided that the above copyright notice and + this permission notice appear in all copies. + + JUCE IS PROVIDED "AS IS" WITHOUT ANY WARRANTY, AND ALL WARRANTIES, WHETHER + EXPRESSED OR IMPLIED, INCLUDING MERCHANTABILITY AND FITNESS FOR PURPOSE, ARE + DISCLAIMED. + + ============================================================================== +*/ + +struct HashMapTest : public UnitTest +{ + HashMapTest() : UnitTest ("HashMap") {} + + void runTest() override + { + doTest ("AddElementsTest"); + doTest ("AccessTest"); + doTest ("RemoveTest"); + doTest ("PersistantMemoryLocationOfValues"); + } + + //============================================================================== + struct AddElementsTest + { + template + static void run (UnitTest& u) + { + AssociativeMap groundTruth; + HashMap hashMap; + + RandomKeys keyOracle (300, 3827829); + Random valueOracle (48735); + + int totalValues = 0; + for (int i = 0; i < 10000; ++i) + { + auto key = keyOracle.next(); + auto value = valueOracle.nextInt(); + + bool contains = (groundTruth.find (key) != nullptr); + u.expectEquals (contains, hashMap.contains (key)); + + groundTruth.add (key, value); + hashMap.set (key, value); + + if (! contains) totalValues++; + + u.expectEquals (hashMap.size(), totalValues); + } + } + }; + + struct AccessTest + { + template + static void run (UnitTest& u) + { + AssociativeMap groundTruth; + HashMap hashMap; + + fillWithRandomValues (hashMap, groundTruth); + + for (auto pair : groundTruth.pairs) + u.expectEquals (hashMap[pair.key], pair.value); + } + }; + + struct RemoveTest + { + template + static void run (UnitTest& u) + { + AssociativeMap groundTruth; + HashMap hashMap; + + fillWithRandomValues (hashMap, groundTruth); + auto n = groundTruth.size(); + + Random r (3827387); + + for (int i = 0; i < 100; ++i) + { + auto idx = r.nextInt (n-- - 1); + auto key = groundTruth.pairs.getReference (idx).key; + + groundTruth.pairs.remove (idx); + hashMap.remove (key); + + u.expect (! hashMap.contains (key)); + + for (auto pair : groundTruth.pairs) + u.expectEquals (hashMap[pair.key], pair.value); + } + } + }; + + // ensure that the addresses of object references don't change + struct PersistantMemoryLocationOfValues + { + struct AddressAndValue { int value; const int* valueAddress; }; + + template + static void run (UnitTest& u) + { + AssociativeMap groundTruth; + HashMap hashMap; + + RandomKeys keyOracle (300, 3827829); + Random valueOracle (48735); + + for (int i = 0; i < 1000; ++i) + { + auto key = keyOracle.next(); + auto value = valueOracle.nextInt(); + + hashMap.set (key, value); + + if (auto* existing = groundTruth.find (key)) + { + // don't change the address: only the value + existing->value = value; + } + else + { + groundTruth.add (key, { value, &hashMap.getReference (key) }); + } + + for (auto pair : groundTruth.pairs) + { + const auto& hashMapValue = hashMap.getReference (pair.key); + + u.expectEquals (hashMapValue, pair.value.value); + u.expect (&hashMapValue == pair.value.valueAddress); + } + } + + auto n = groundTruth.size(); + Random r (3827387); + + for (int i = 0; i < 100; ++i) + { + auto idx = r.nextInt (n-- - 1); + auto key = groundTruth.pairs.getReference (idx).key; + + groundTruth.pairs.remove (idx); + hashMap.remove (key); + + for (auto pair : groundTruth.pairs) + { + const auto& hashMapValue = hashMap.getReference (pair.key); + + u.expectEquals (hashMapValue, pair.value.value); + u.expect (&hashMapValue == pair.value.valueAddress); + } + } + } + }; + + //============================================================================== + template + void doTest (const String& testName) + { + beginTest (testName); + + Test::template run (*this); + Test::template run (*this); + Test::template run (*this); + } + + //============================================================================== + template + struct AssociativeMap + { + struct KeyValuePair { KeyType key; ValueType value; }; + + ValueType* find (KeyType key) + { + auto n = pairs.size(); + + for (int i = 0; i < n; ++i) + { + auto& pair = pairs.getReference (i); + + if (pair.key == key) + return &pair.value; + } + + return nullptr; + } + + void add (KeyType key, ValueType value) + { + if (ValueType* v = find (key)) + *v = value; + else + pairs.add ({key, value}); + } + + int size() const { return pairs.size(); } + + Array pairs; + }; + + template + static void fillWithRandomValues (HashMap& hashMap, AssociativeMap& groundTruth) + { + RandomKeys keyOracle (300, 3827829); + Random valueOracle (48735); + + for (int i = 0; i < 10000; ++i) + { + auto key = keyOracle.next(); + auto value = valueOracle.nextInt(); + + groundTruth.add (key, value); + hashMap.set (key, value); + } + } + + //============================================================================== + template + class RandomKeys + { + public: + RandomKeys (int maxUniqueKeys, int seed) : r (seed) + { + for (int i = 0; i < maxUniqueKeys; ++i) + keys.add (generateRandomKey (r)); + } + + const KeyType& next() + { + int i = r.nextInt (keys.size() - 1); + return keys.getReference (i); + } + private: + static KeyType generateRandomKey (Random&); + + Random r; + Array keys; + }; +}; + +template <> int HashMapTest::RandomKeys ::generateRandomKey (Random& rnd) { return rnd.nextInt(); } +template <> void* HashMapTest::RandomKeys::generateRandomKey (Random& rnd) { return reinterpret_cast (rnd.nextInt64()); } + +template <> String HashMapTest::RandomKeys::generateRandomKey (Random& rnd) +{ + String str; + + int len = rnd.nextInt (8)+1; + for (int i = 0; i < len; ++i) + str += static_cast (rnd.nextInt (95) + 32); + + return str; +} + +static HashMapTest hashMapTest; diff --git a/modules/juce_core/juce_core.cpp b/modules/juce_core/juce_core.cpp index 6a4fa7547a..7d10dfa25e 100644 --- a/modules/juce_core/juce_core.cpp +++ b/modules/juce_core/juce_core.cpp @@ -232,6 +232,11 @@ namespace juce #include "network/juce_URL.cpp" #include "network/juce_WebInputStream.cpp" +//============================================================================== +#if JUCE_UNIT_TESTS +#include "containers/juce_HashMap_test.cpp" +#endif + //============================================================================== /* As the very long class names here try to explain, the purpose of this code is to cause