diff --git a/src/proc/asset/entry-id.hpp b/src/proc/asset/entry-id.hpp index 7667c8ec6..445597fb8 100644 --- a/src/proc/asset/entry-id.hpp +++ b/src/proc/asset/entry-id.hpp @@ -82,12 +82,16 @@ namespace asset { * conjunction with LUID. How to create a LuidH instance, if not generating * a new random value. How to make EntryID and asset::Ident interchangeable, /////////TICKET #739 * which would require both to yield the same hash values.... + * @warning there is a weakness in boost::hash for strings of running numbers, causing + * collisions already for a small set with less than 100000 entries. + * To ameliorate the problem, we hash the symbol twice /////////TICKET #865 * @warning this code isn't portable and breaks if sizeof(size_t) < sizeof(void*) */ inline LuidH buildHash (string const& sym, HashVal seed =0) { boost::hash_combine(seed, sym); + boost::hash_combine(seed, sym); ////////////////////////TICKET #865 lumiera_uid tmpLUID; lumiera_uid_set_ptr (&tmpLUID, reinterpret_cast (seed)); return reinterpret_cast (tmpLUID); @@ -173,6 +177,11 @@ namespace asset { * EntryID template share a common baseclass, usable for type erased common registration. * @todo currently storing the symbolic-ID as string. It should be a lib::Symbol, * but this isn't possible unless we use a symbol table. //////TICKET #158 + * @warning there is a weakness in boost::hash applied to strings. EntryID by default + * generates symbolic IDs from a type prefix plus a running number, which causes + * collisions already with less than 100000 entries. + * @todo As a temporary workaround, we hash the symbolic ID twice, but we should look + * into using a better hash function ////////////TICKET #865 * * @see mobject::session::Track */ diff --git a/tests/40components.tests b/tests/40components.tests index da70c1ce4..9baa7e6b5 100644 --- a/tests/40components.tests +++ b/tests/40components.tests @@ -356,6 +356,11 @@ return: 0 END +TEST "HashGenerator_test" HashGenerator_test < +#include +#include +#include +#include + +using boost::lexical_cast; +using util::contains; +using std::string; +using std::cout; +using std::endl; + + +namespace lib { +namespace test{ + + + + + + /*************************************************************************** + * @test cover various detail aspects regarding hash value generation + * - weakness of boost::hash + * + * @see HashIndexed_test + * @see EntryID_test + */ + class HashGenerator_test : public Test + { + + virtual void + run (Arg) + { + demonstrate_boost_hash_weakness(); + } + + + /** @test demonstrate a serious weakness of boost::hash for strings. + * When hashing just the plain string representation of integers, + * we get collisions already with small numbers below 100000. + * This is counter-intuitive, as the generated hash values + * are 17 digits long and could span much wider scale. + * + * This problem is especially dangerous when storing objects keyed + * by a string-id, which is generated from running numbers. + */ + void + demonstrate_boost_hash_weakness () + { + typedef boost::hash BoostStringHasher; + typedef std::map StringsTable; + + BoostStringHasher hashFunction; + StringsTable hashValues; + string prefix = "Entry."; + uint collisions(0); + for (uint i=0; i<100000; ++i) + { + string candidate = prefix + lexical_cast (i); + size_t hashVal = hashFunction(candidate); + + if (contains (hashValues, hashVal)) + { + ++collisions; + string other = hashValues[hashVal]; + cout << "Duplicate at "<< i << endl; + cout << "existing--->" << other << endl; + cout << "new-------->" << candidate << endl; + + size_t exHash = hashFunction(other); + size_t newHash = hashFunction(candidate); + cout << "hash-ex---->" << exHash << endl; + cout << "hash_new--->" << newHash << endl; + } + hashValues[hashVal] = candidate; + } + CHECK (0 < collisions, "boost::hash for strings is expected to produce collisions"); + } + + }; + + + /** Register this test class... */ + LAUNCHER (HashGenerator_test, "unit common"); + + +}} // namespace lib::test diff --git a/tests/lib/time/quantiser-basics-test.cpp b/tests/lib/time/quantiser-basics-test.cpp index b503e3d29..cca585491 100644 --- a/tests/lib/time/quantiser-basics-test.cpp +++ b/tests/lib/time/quantiser-basics-test.cpp @@ -88,7 +88,7 @@ namespace test{ FixedFrameQuantiser fixQ(25); uint frames = (rand() % MAX_FRAMES); - FSecs dirt = (F25 / (rand() % DIRT_GRAIN)); + FSecs dirt = (F25 / (1 + rand() % DIRT_GRAIN)); Time rawTime (dirt + frames*F25);