[esan] Add generic resizing hashtable

[bruening]

Summary

Adds a new, generic, resizing hashtable data structure for use by esan
tools. No existing sanitizer hashtable is suitable for the use case for
most esan tools: we need non-fixed-size tables, parameterized keys and
payloads, and write access to payloads. The new hashtable uses either
simple internal or external mutex locking and supports pointer types using
overloads to de-reference and allow for custom hash and comparision
operators. The focus is on functionality, not performance, to catalyze
creation of a variety of tools. We can optimize the more successful tools
later.

Adds a test of the data structure.

[aizatsky]

lib/esan/esan_hashtable.h

24

Key pointer types...

24

I actually wonder if this is a functionality you want to build in rather then provide through type traits.

Like this:

template <typename KeyTy, typename DataTy, typename KeyTraits = DefaultTraits<KeyTy>>

And later something like:

KeyTraits::hash(key)
KeyTraits::eq(key1, key2).

42

any particular reason why not

DataTy* lookup(KeyTy Key) ?

Seems to me it would always save a declaration line for clients.

42

const

45

const

66

const this and other non-changed fields.

[bruening]

lib/esan/esan_hashtable.h

24

Could you elaborate on what the pros and cons of your proposal are? I assume you mean that we define DefaultTraits to invoke operator== and operator size_t() and the point here is to let the user separate hashing from casting and key equality from some other equality -- though the existing constructor takes in an optional hash function for that purpose, and I'm not sure equality would need to be separated.

42

any particular reason why not
DataTy* lookup(KeyTy Key) ?

Returning a pointer requires external synchronization. An instantiation with DataTy==int will want an int to be returned by value, and there's no sentinel we can use to indicate failure.

42

const

I ended up removing my initial const because of the potential lock acquisition: the mutex will change state. I don't think LLVM supports the C++11 mutable keyword?

45

Same thing here. I can at least put a comment about it.

[aizatsky]

lib/esan/esan_hashtable.h

24

Pros:

• makes HashTable simpler
• moves type-based decision into a much-much simpler class
• type-based decision can be made explicit. Consider:
  
  HashTable<int*, int> vs HashTable<int*, int, DerefPointers>

I would also suggest to move HashFunc to the trait as well. This would improve performance of the hashing.

The only const that I see is that you need to define a default trait struct. But that looks more like a pro to me :)

28

Again, I don't like that there's a conditional branch throughout the class based on the field.

The most logical way to me would be to define HashTable without any synchronization (i.e. ExternalLock = true), and create a SynchronizedHashTable : public HashTable with corresponding overrides.

[bruening]

lib/esan/esan_hashtable.h

24

I agree that the table would be improved to move the hash function and comparison function into template parameters, but I'd prefer to not combine them, which forces a user who wants to set just one to define both.

I still like a default that de-references pointers, but after removing the payload-freeing function parameter and relying on the user using a smart pointer to free, and considering that in C++ the payload is easily copied into the hashtable entry structure via templates (unlike C where indirection via a pointer into the heap is the typical solution), which means that the payload is less likely to be a pointer, and references means that the key is also less likely to be a pointer (yes still more of a C programmer than a C++ programmer), I'm fine removing it.

28

The problem is that inheritance means we pay virtual dispatch costs, while a container (i.e., SynchronizedHashTable contains a HashTable inside it) just means a lot of duplicated boilerplate to wrap everything. I instead made the ExternalLock a template parameter and thus constant at compile time, which eliminates the conditional branches.

[bruening]

Makes the table more like a classic C++ container:
+ Moves the hash function pointer to be a functor template parameter
+ Adds a functor template parameter for the comparision function
+ Moves ExternalLock to be a template paramter to eliminate conditional branches
+ Eliminates the default pointer deref
+ Removes the payload-freeing function pointer in favor of calling the destructor

lib/esan/esan_hashtable.h

+//===-- esan_hashtable.h ----------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is a part of EfficiencySanitizer, a family of performance tuners.
+//
+// Generic resizing hashtable.
+//===----------------------------------------------------------------------===//
+
+#include "sanitizer_common/sanitizer_allocator_internal.h"
+#include "sanitizer_common/sanitizer_internal_defs.h"
+#include "sanitizer_common/sanitizer_mutex.h"
+#include <stddef.h>
+
+namespace __esan {
+
+// A simple resizing and mutex-locked hashtable.
+// KeyTy must have an operator== and an operator size_t() (for hashing).
+// Pointer types will be automatically de-referenced prior to either operator==
+// or a size_t hash cast.
+// By default all operations are internally-synchronized with a mutex, with no
+// synchronization for payloads once hashtable functions return.  If
+// ExternalLock is set to true, the caller should call the lock() and unlock()
+// routines around all hashtable operations and subsequent manipulation of
+// payloads.
+template <typename KeyTy, typename DataTy> class HashTable {
+ public:
+  typedef size_t (*HashFuncTy)(KeyTy Key);
+  typedef void (*FreeDataTy)(DataTy Data);
+  // InitialCapacity must be a power of 2.
+  // ResizeFactor must be between 1 and 99 and indicates the
+  // maximum percentage full that the table should ever be.
+  HashTable(u32 InitialCapacity = 2048, u32 ResizeFactor = 70, 
+            bool ExternalLock = false, FreeDataTy FreeFunc = nullptr,
+            HashFuncTy HashFunc = hashDefault);
+  ~HashTable();
+  bool lookup(KeyTy Key, DataTy &Payload);
+  bool add(KeyTy Key, DataTy Payload);
+  bool remove(KeyTy Key);
+  u32 size();
+  // If the table is internally-synchronized, this lock must not be held
+  // while a hashtable function is called as it will deadlock: the lock
+  // is not recursive.  This is meant for use with externally-synchronized
+  // tables.
+  void lock();
+  void unlock();
+
+ private:
+  void resize();
+  static size_t hashDefault(KeyTy Key);
+
+  struct HashEntry {
+    KeyTy Key;
+    DataTy Payload;
+    HashEntry *Next;
+  };
+
+  HashEntry **Table;
+  u32 Capacity;
+  u32 Entries;
+  u32 ResizeFactor;
+  BlockingMutex Mutex;
+  bool ExternalLock;
+  HashFuncTy HashFunc;
+  FreeDataTy FreeFunc;
+};
+
+//===----------------------------------------------------------------------===//
+// Handle both pointers and values
+//===----------------------------------------------------------------------===//
+
+template<typename T> T& deRef(T Var) {
+  // The type is not a pointer, so just use it.
+  return Var;
+}
+
+template<typename T> T& deRef(T *Var) {
+  // De-reference a pointer type so that its operator== and operator size_t()
+  // will be used.
+  return *Var;
+}
+
+//===----------------------------------------------------------------------===//
+// Hashtable implementation
+//===----------------------------------------------------------------------===//
+
+template <typename KeyTy, typename DataTy>
+HashTable<KeyTy, DataTy>::HashTable(u32 InitialCapacity, u32 ResizeFactor, 
+                                    bool ExternalLock, FreeDataTy FreeFunc,
+                                    HashFuncTy HashFunc) :
+    Capacity(InitialCapacity), Entries(0), ResizeFactor(ResizeFactor),
+    ExternalLock(ExternalLock), HashFunc(HashFunc), FreeFunc(FreeFunc) {
+  CHECK(IsPowerOfTwo(Capacity));
+  CHECK(ResizeFactor >= 1 && ResizeFactor <= 99);
+  Table = (HashEntry**)InternalAlloc(Capacity*sizeof(HashEntry*));
+  internal_memset(Table, 0, Capacity*sizeof(HashEntry*));
+}
+
+template <typename KeyTy, typename DataTy>
+HashTable<KeyTy, DataTy>::~HashTable() {
+  for (u32 i = 0; i < Capacity; ++i) {
+    HashEntry *Entry = Table[i];
+    while (Entry != nullptr) {
+      HashEntry *Next = Entry->Next;
+      if (FreeFunc)
+        FreeFunc(Entry->Payload);
+      InternalFree(Entry);
+      Entry = Next;
+    }
+  }
+  InternalFree(Table);
+}
+
+template <typename KeyTy, typename DataTy>
+size_t HashTable<KeyTy, DataTy>::hashDefault(KeyTy Key) {
+  return (size_t)deRef(Key);
+}
+
+template <typename KeyTy, typename DataTy>
+u32 HashTable<KeyTy, DataTy>::size() {
+  u32 Res;
+  if (!ExternalLock)
+    Mutex.Lock();
+  Res = Entries;
+  if (!ExternalLock)
+    Mutex.Unlock();
+  return Res;
+}
+
+template <typename KeyTy, typename DataTy>
+bool HashTable<KeyTy, DataTy>::lookup(KeyTy Key, DataTy &Payload) {
+  if (!ExternalLock)
+    Mutex.Lock();
+  bool Found = false;
+  size_t Hash = HashFunc(Key) % Capacity;
+  HashEntry *Entry = Table[Hash];
+  for (; Entry != nullptr; Entry = Entry->Next) {
+    if (deRef(Entry->Key) == deRef(Key)) {
+      Payload = Entry->Payload;
+      Found = true;
+      break;
+    }
+  }
+  if (!ExternalLock)
+    Mutex.Unlock();
+  return Found;
+}
+
+template <typename KeyTy, typename DataTy>
+void HashTable<KeyTy, DataTy>::resize() {
+  if (!ExternalLock)
+    Mutex.CheckLocked();
+  size_t OldCapacity = Capacity;
+  HashEntry **OldTable = Table;
+  Capacity *= 2;
+  Table = (HashEntry**)InternalAlloc(Capacity*sizeof(HashEntry*));
+  internal_memset(Table, 0, Capacity*sizeof(HashEntry*));
+  // Re-hash
+  for (u32 i = 0; i < OldCapacity; ++i) {
+    HashEntry *OldEntry = OldTable[i];
+    while (OldEntry != nullptr) {
+      HashEntry *Next = OldEntry->Next;
+      size_t Hash = HashFunc(OldEntry->Key) % Capacity;
+      OldEntry->Next = Table[Hash];
+      Table[Hash] = OldEntry;
+      OldEntry = Next;
+    }
+  }
+}
+
+template <typename KeyTy, typename DataTy>
+bool HashTable<KeyTy, DataTy>::add(KeyTy Key, DataTy Payload) {
+  if (!ExternalLock)
+    Mutex.Lock();
+  bool Exists = false;
+  size_t Hash = HashFunc(Key) % Capacity;
+  HashEntry *Entry = Table[Hash];
+  for (; Entry != nullptr; Entry = Entry->Next) {
+    if (deRef(Entry->Key) == deRef(Key)) {
+      Exists = true;
+      break;
+    }
+  }
+  if (!Exists) {
+    Entries++;
+    if (Entries*100 >= Capacity*ResizeFactor) {
+      resize();
+      Hash = HashFunc(Key) % Capacity;
+    }
+    HashEntry *Add = (HashEntry*)InternalAlloc(sizeof(*Add));
+    Add->Key = Key;
+    Add->Payload = Payload;
+    Add->Next = Table[Hash];
+    Table[Hash] = Add;
+  }
+  if (!ExternalLock)
+    Mutex.Unlock();
+  return !Exists;
+}
+
+template <typename KeyTy, typename DataTy>
+bool HashTable<KeyTy, DataTy>::remove(KeyTy Key) {
+  if (!ExternalLock)
+    Mutex.Lock();
+  bool Found = false;
+  size_t Hash = HashFunc(Key) % Capacity;
+  HashEntry *Entry = Table[Hash];
+  HashEntry *Prev = nullptr;
+  for (; Entry != nullptr; Prev = Entry, Entry = Entry->Next) {
+    if (deRef(Entry->Key) == deRef(Key)) {
+      Found = true;
+      Entries--;
+      if (Prev == nullptr)
+        Table[Hash] = Entry->Next;
+      else
+        Prev->Next = Entry->Next;
+      if (FreeFunc)
+        FreeFunc(Entry->Payload);
+      InternalFree(Entry);
+      break;
+    }
+  }
+  if (!ExternalLock)
+    Mutex.Unlock();
+  return Found;
+}
+
+template <typename KeyTy, typename DataTy>
+void HashTable<KeyTy, DataTy>::lock() {
+  Mutex.Lock();
+}
+
+template <typename KeyTy, typename DataTy>
+void HashTable<KeyTy, DataTy>::unlock() {
+  Mutex.Unlock();
+}
+
+} // namespace __esan

test/esan/Unit/hashtable.cpp

+// RUN: %clangxx_unit -O0 %s -o %t 2>&1
+// RUN: %env_esan_opts="record_snapshots=0" %run %t 2>&1 | FileCheck %s
+
+#include "esan/esan_hashtable.h"
+#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+class MyData {
+ public:
+  MyData(const char *Str) { Buf = strdup(Str); }
+  ~MyData() { free(Buf); }
+  bool operator==(MyData &Cmp) { return strcmp(Buf, Cmp.Buf) == 0; }
+  operator size_t() const {
+    size_t Res = 0;
+    for (int i = 0; i < strlen(Buf); ++i)
+      Res ^= Buf[i];
+    return Res;
+  }
+  char *Buf;
+};
+
+void freeData(MyData *Data) {
+  fprintf(stderr, "Deleting %s.\n", Data->Buf);
+  delete Data;
+}
+
+int main()
+{
+  __esan::HashTable<int, int> IntTable;
+  assert(IntTable.size() == 0);
+  bool Added = IntTable.add(4, 42);
+  assert(Added);
+  assert(!IntTable.add(4, 42));
+  assert(IntTable.size() == 1);
+  int Value;
+  bool Found = IntTable.lookup(4, Value);
+  assert(Found && Value == 42);
+  assert(!IntTable.remove(5));
+  assert(IntTable.remove(4));
+
+  __esan::HashTable<int, MyData*> DataTable(4, 50, false, freeData);
+  MyData *NewData = new MyData("mystring");
+  Added = DataTable.add(4, NewData);
+  assert(Added);
+  MyData *FoundData;
+  Found = DataTable.lookup(4, FoundData);
+  assert(Found && strcmp(FoundData->Buf, "mystring") == 0);
+  assert(!DataTable.remove(5));
+  assert(DataTable.remove(4));
+  // Test resize.
+  for (int i = 0; i < 4; ++i) {
+    NewData = new MyData("delete-at-end");
+    Added = DataTable.add(i+1, NewData);
+    assert(Added);
+    assert(!DataTable.add(i+1, NewData));
+  }
+  for (int i = 0; i < 4; ++i) {
+    Found = DataTable.lookup(i+1, FoundData);
+    assert(Found && strcmp(FoundData->Buf, "delete-at-end") == 0);
+  }
+
+  fprintf(stderr, "All checks passed.\n");
+  return 0;
+}
+// CHECK:      Deleting mystring.
+// CHECK-NEXT: All checks passed.
+// CHECK-NEXT: Deleting delete-at-end.
+// CHECK-NEXT: Deleting delete-at-end.
+// CHECK-NEXT: Deleting delete-at-end.
+// CHECK-NEXT: Deleting delete-at-end.