kernel/EntryCache: Use a hash table with atomics for insertions.

This massively cuts down on lock contention in Add(), since insertions
only acquire a write-lock in the case where a generation rolls over,
same as Lookup() does.

"git status" in buildtools, cold disk cache in a 4-core VM, seems about
the same, maybe slightly slower (~0.5s seemed typical, out of 20-21s),
while with a hot disk cache it's much faster: ~9.8s -> ~2.4s. Compile
performance seemed about the same.

Change-Id: Ia73f35fbbad3b3ac9ed783ea38cb8e2cb9818b5b
Reviewed-on: https://review.haiku-os.org/c/haiku/+/9580
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: waddlesplash <[email protected]>

headers/private/kernel/util/AtomicsHashTable.h

@@ -0,0 +1,79 @@
+/*
+ * Copyright 2025, Haiku, Inc. All rights reserved.
+ * Distributed under the terms of the MIT License.
+ */
+#ifndef _KERNEL_UTIL_ATOMICS_HASH_TABLE_H
+#define _KERNEL_UTIL_ATOMICS_HASH_TABLE_H
+
+
+#include <util/OpenHashTable.h>
+#include <util/atomic.h>
+
+
+// AtomicsHashTable extends BOpenHashTable with some atomic operations.
+
+template<typename Definition, bool AutoExpand = true,
+	bool CheckDuplicates = false>
+class AtomicsHashTable : public BOpenHashTable<Definition,
+	AutoExpand, CheckDuplicates> {
+public:
+	typedef BOpenHashTable<Definition, AutoExpand, CheckDuplicates> HashTable;
+
+	typedef typename Definition::KeyType KeyType;
+	typedef typename Definition::ValueType ValueType;
+
+	AtomicsHashTable()
+		: HashTable() {}
+
+	AtomicsHashTable(const Definition& definition)
+		: HashTable(definition) {}
+
+	/*! \brief Inserts a value atomically.
+
+		If there's another item with an identical key, the new value will not
+		be inserted, and that value will be returned instead. If there's no
+		other value with the same key, the passed value will be inserted,
+		and NULL will be returned.
+
+		The caller is responsible for ensuring that no Remove()s or Resize()s
+		are invoked concurrently with this method.
+	*/
+	ValueType* InsertAtomic(ValueType* value)
+	{
+		KeyType key = HashTable::fDefinition.Key(value);
+		size_t index = HashTable::fDefinition.Hash(value) & (HashTable::fTableSize - 1);
+		HashTable::_Link(value) = NULL;
+
+		ValueType** link = &HashTable::fTable[index];
+		while (true) {
+			ValueType* existing = atomic_pointer_get(link);
+			if (existing == NULL) {
+				existing = atomic_pointer_test_and_set(link, value, existing);
+				if (existing == NULL) {
+					size_t& count = HashTable::fItemCount;
+					sizeof(size_t) == 4 ?
+						atomic_add((int32*)&count, 1) :
+						atomic_add64((int64*)&count, 1);
+					return NULL;
+				}
+			}
+
+			if (HashTable::fDefinition.Compare(key, existing))
+				return existing;
+
+			link = &HashTable::_Link(existing);
+		}
+	}
+
+	bool ResizeIfNeeded()
+	{
+		size_t resizeNeeded = HashTable::ResizeNeeded();
+		if (resizeNeeded == 0)
+			return true;
+
+		return HashTable::_Resize(resizeNeeded);
+	}
+};
+
+
+#endif	// _KERNEL_UTIL_ATOMICS_HASH_TABLE_H

src/system/kernel/fs/EntryCache.cpp

@@ -122,35 +122,24 @@ EntryCache::Add(ino_t dirID, const char* name, ino_t nodeID, bool missing)
 	entry->generation = -1;
 	memcpy(entry->name, name, nameLen + 1);
 
-	WriteLocker writeLocker(fLock);
+	ReadLocker readLocker(fLock);
 
 	if (fGenerationCount == 0) {
 		free(entry);
 		return B_NO_MEMORY;
 	}
 
-	EntryCacheEntry* existingEntry = fEntries.Lookup(key);
+	EntryCacheEntry* existingEntry = fEntries.InsertAtomic(entry);
 	if (existingEntry != NULL) {
 		free(entry);
 		entry = existingEntry;
 
 		entry->node_id = nodeID;
 		entry->missing = missing;
-		if (entry->generation != fCurrentGeneration) {
-			if (entry->index >= 0) {
-				fGenerations[entry->generation].entries[entry->index] = NULL;
-
-				writeLocker.Detach();
-				_AddEntryToCurrentGeneration(entry);
-			}
-		}
-		return B_OK;
 	}
 
-	fEntries.Insert(entry);
+	readLocker.Detach();
-
+	_AddEntryToCurrentGeneration(entry, entry == existingEntry);
-	writeLocker.Detach();
-	_AddEntryToCurrentGeneration(entry);
 	return B_OK;
 }
 
@@ -196,48 +185,11 @@ EntryCache::Lookup(ino_t dirID, const char* name, ino_t& _nodeID,
 	if (entry == NULL)
 		return false;
 
-	const int32 oldGeneration = atomic_get_and_set(&entry->generation,
-		fCurrentGeneration);
-	if (oldGeneration == fCurrentGeneration || entry->index < 0) {
-		// The entry is already in the current generation or is being moved to
-		// it by another thread.
-		_nodeID = entry->node_id;
-		_missing = entry->missing;
-		return true;
-	}
-
-	// remove from old generation array
-	fGenerations[oldGeneration].entries[entry->index] = NULL;
-	entry->index = kEntryNotInArray;
-
-	// add to the current generation
-	const int32 index = atomic_add(&fGenerations[fCurrentGeneration].next_index, 1);
-	if (index < fGenerations[fCurrentGeneration].entries_size) {
-		fGenerations[fCurrentGeneration].entries[index] = entry;
-		entry->index = index;
-		_nodeID = entry->node_id;
-		_missing = entry->missing;
-		return true;
-	}
-
-	// The current generation is full, so we probably need to clear the oldest
-	// one to make room. We need the write lock for that.
-	readLocker.Unlock();
-	WriteLocker writeLocker(fLock);
-
-	if (entry->index == kEntryRemoved) {
-		// the entry has been removed in the meantime
-		writeLocker.Unlock();
-		free(entry);
-		return false;
-	}
-
 	_nodeID = entry->node_id;
 	_missing = entry->missing;
 
-	writeLocker.Detach();
+	readLocker.Detach();
-	_AddEntryToCurrentGeneration(entry);
+	return _AddEntryToCurrentGeneration(entry, true);
-	return true;
 }
 
 
@@ -257,18 +209,56 @@ EntryCache::DebugReverseLookup(ino_t nodeID, ino_t& _dirID)
 }
 
 
-void
+bool
-EntryCache::_AddEntryToCurrentGeneration(EntryCacheEntry* entry)
+EntryCache::_AddEntryToCurrentGeneration(EntryCacheEntry* entry, bool move)
 {
-	WriteLocker locker(fLock, true);
+	ReadLocker readLocker(fLock, true);
+
+	if (move) {
+		const int32 oldGeneration = atomic_get_and_set(&entry->generation,
+			fCurrentGeneration);
+		if (oldGeneration == fCurrentGeneration || entry->index < 0) {
+			// The entry is already in the current generation or is being moved to
+			// it by another thread.
+			return true;
+		}
+
+		// remove from old generation array
+		fGenerations[oldGeneration].entries[entry->index] = NULL;
+		entry->index = kEntryNotInArray;
+	} else {
+		entry->generation = fCurrentGeneration;
+	}
+
+	// add to the current generation
+	int32 index = atomic_add(&fGenerations[fCurrentGeneration].next_index, 1);
+	if (index < fGenerations[fCurrentGeneration].entries_size) {
+		fGenerations[fCurrentGeneration].entries[index] = entry;
+		entry->index = index;
+		return true;
+	}
+
+	// The current generation is full, so we probably need to clear the oldest
+	// one to make room. We need the write lock for that.
+	readLocker.Unlock();
+	WriteLocker writeLocker(fLock);
+
+	if (entry->index == kEntryRemoved) {
+		// the entry has been removed in the meantime
+		writeLocker.Unlock();
+		free(entry);
+		return false;
+	}
 
 	// the generation might not be full yet
-	int32 index = fGenerations[fCurrentGeneration].next_index++;
+	index = fGenerations[fCurrentGeneration].next_index++;
 	if (index < fGenerations[fCurrentGeneration].entries_size) {
 		fGenerations[fCurrentGeneration].entries[index] = entry;
 		entry->generation = fCurrentGeneration;
 		entry->index = index;
-		return;
+
+		fEntries.ResizeIfNeeded();
+		return true;
 	}
 
 	// we have to clear the oldest generation
@@ -280,7 +270,7 @@ EntryCache::_AddEntryToCurrentGeneration(EntryCacheEntry* entry)
 			continue;
 
 		fGenerations[newGeneration].entries[i] = NULL;
-		fEntries.Remove(otherEntry);
+		fEntries.RemoveUnchecked(otherEntry);
 
 		otherEntry->hash_link = entriesToFree;
 		entriesToFree = otherEntry;
@@ -294,10 +284,12 @@ EntryCache::_AddEntryToCurrentGeneration(EntryCacheEntry* entry)
 	entry->index = 0;
 
 	// free the old entries
-	locker.Unlock();
+	writeLocker.Unlock();
 	while (entriesToFree != NULL) {
 		EntryCacheEntry* next = entriesToFree->hash_link;
 		free(entriesToFree);
 		entriesToFree = next;
 	}
+
+	return true;
 }

src/system/kernel/fs/EntryCache.h

@@ -9,20 +9,23 @@
 #include <stdlib.h>
 
 #include <util/AutoLock.h>
+#include <util/AtomicsHashTable.h>
 #include <util/DoublyLinkedList.h>
-#include <util/OpenHashTable.h>
 #include <util/StringHash.h>
 
 
 struct EntryCacheKey {
-	EntryCacheKey(ino_t dirID, const char* name)
+	EntryCacheKey(ino_t dirID, const char* name, const uint32* _hash = NULL)
 		:
 		dir_id(dirID),
 		name(name)
 	{
+		if (_hash == NULL) {
+			// We cache the hash value, so we compute it before holding any locks.
 			hash = Hash(dirID, name);
-			// We cache the hash value, so we can easily compute it before
+		} else {
-			// holding any locks.
+			hash = *_hash;
+		}
 	}
 
 	static uint32 Hash(ino_t dirID, const char* name)
@@ -74,6 +77,11 @@ struct EntryCacheHashDefinition {
 		return value->hash;
 	}
 
+	EntryCacheKey Key(const EntryCacheEntry* value) const
+	{
+		return EntryCacheKey(value->dir_id, value->name, &value->hash);
+	}
+
 	bool Compare(const EntryCacheKey& key, const EntryCacheEntry* value) const
 	{
 		if (key.hash != value->hash)
@@ -107,12 +115,12 @@ public:
 			const char*			DebugReverseLookup(ino_t nodeID, ino_t& _dirID);
 
 private:
-			typedef BOpenHashTable<EntryCacheHashDefinition> EntryTable;
+			typedef AtomicsHashTable<EntryCacheHashDefinition> EntryTable;
 			typedef DoublyLinkedList<EntryCacheEntry> EntryList;
 
 private:
-			void				_AddEntryToCurrentGeneration(
+			bool				_AddEntryToCurrentGeneration(
-									EntryCacheEntry* entry);
+									EntryCacheEntry* entry, bool move);
 
 private:
 			rw_lock				fLock;