From 6c07dcb839ea694995733dcea3b915eb92d02f0b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 17:26:50 +0000 Subject: [PATCH 01/18] perf(quartz): drop copy-on-write from linuxX64's LargeCache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LocalCache's linuxX64 store kept a LinkedHashMap inside an AtomicReference and replaced it wholesale on every write, so each put was O(n) in the size of the cache and filling it was O(n^2). It was not thread-safe either: the read-copy-write was not a CAS loop, so concurrent writers silently dropped each other's entries. Replace it with a mutable map guarded by PlatformLock plus a lazily rebuilt read snapshot. Point operations (get/put/remove/containsKey/size) are O(1) under the lock; bulk operations run against a point-in-time copy rebuilt at most once per write epoch, which also keeps caller-supplied lambdas out of the critical section — PlatformLock is not reentrant here and LocalCache predicates call back into the cache. Two behaviour fixes fall out of matching the JVM actual's putIfAbsent: createIfAbsent now reports true only when this call inserted (it previously returned get(key) != null, which also reported true when another thread had just created the entry), and getOrCreate publishes atomically. ConcurrentHashCache.linux gets the same treatment. Its only caller, CachingEventDecoder, writes once per event arriving from a relay, so the per-write map rebuild was the worst-placed copy of the three. None of this was caught because no CI job compiled or ran linuxX64. Add LargeCacheTest to commonTest as a cross-target contract for the ~40 methods each actual reimplements by hand, a linuxTest suite covering the concurrency this actual now has to get right on its own, and a CI leg that runs both on Linux Native. That leg is scoped to the cache and concurrency packages: the full linuxX64Test suite is 3,490 tests with 78 pre-existing failures, nearly all TODO() stubs in linux actuals that were never written (MLS crypto, the SQLite driver, NIP-44, Bolt12). Filling those in is its own project; the filter keeps the job meaningful and green, and widening it later is one line. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HxQ1QuyzSkR38iFHbREjoS --- .github/workflows/build.yml | 70 ++++ .../quartz/utils/cache/LargeCacheTest.kt | 319 ++++++++++++++++++ .../utils/cache/ConcurrentHashCache.linux.kt | 33 +- .../quartz/utils/cache/LargeCache.linux.kt | 169 ++++++++-- .../utils/cache/LargeCacheConcurrencyTest.kt | 120 +++++++ .../cache/LargeCacheRangeFallbackTest.kt | 73 ++++ .../utils/concurrent/ConcurrentMap.native.kt | 10 +- 7 files changed, 742 insertions(+), 52 deletions(-) create mode 100644 quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheTest.kt create mode 100644 quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheConcurrencyTest.kt create mode 100644 quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheRangeFallbackTest.kt diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 06d0b541d1..c130551bce 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -194,6 +194,76 @@ jobs: name: geode Test Reports path: geode/build/reports + # linuxX64 is the only target whose LargeCache / ConcurrentHashCache actuals are + # hand-written concurrent maps rather than a delegation to a platform concurrent + # collection, and until this job existed nothing ran them: the target was compiled + # by no CI leg at all. That is how a copy-on-write LargeCache with O(n) writes and a + # non-atomic read-copy-write (concurrent writers silently dropped entries) sat in + # the tree unnoticed. + # + # Scoped to the cache/concurrency packages on purpose. The full linuxX64Test suite + # is 3,490 tests with 78 pre-existing failures, essentially all of them `TODO()` + # stubs in linux actuals that were never written — MLS crypto, the SQLite driver, + # NIP-44, Bolt12 — plus two URL-handling divergences. Filling those in is its own + # project; gating PRs on them today would just mean a permanently red job. The + # filter keeps the leg meaningful and green, and widening it is a one-line change + # once the native actuals land. + # + # This still compiles and links the whole module for linuxX64, so a commonMain or + # commonTest source that reaches for a JVM-only API fails here too — on a target + # with no Foundation to fall back on the way Apple has. + test-quartz-linux-native: + needs: lint + runs-on: ubuntu-latest + timeout-minutes: 45 + steps: + - name: Checkout code + uses: actions/checkout@v7 + + - name: Set up JDK 21 + uses: actions/setup-java@v6.0.0 + with: + distribution: 'temurin' + java-version: 21 + + - name: Set up Gradle + uses: gradle/actions/setup-gradle@v6 + with: + cache-read-only: ${{ github.ref != 'refs/heads/main' }} + + # The Kotlin/Native toolchain (compiler distribution + LLVM + the sysroot) lands + # in ~/.konan, which setup-gradle does not cache. Without this the job re-downloads + # well over a gigabyte on every run. Keyed on the version catalog so a Kotlin bump + # re-populates it. + - name: Cache Kotlin/Native toolchain + uses: actions/cache@v4 + with: + path: ~/.konan + key: konan-${{ runner.os }}-${{ hashFiles('gradle/libs.versions.toml') }} + restore-keys: konan-${{ runner.os }}- + + - name: Test Quartz caches on Linux Native + run: | + ./gradlew :quartz:linuxX64Test \ + --tests "com.vitorpamplona.quartz.utils.cache.*" \ + --tests "com.vitorpamplona.quartz.utils.concurrent.*" + + - name: Linux Native Test Report + uses: mikepenz/action-junit-report@a9170d5795813c01ab4901ffb045b52bab4ab09d # v6.5.0 + if: always() + with: + report_paths: 'quartz/build/test-results/linuxX64Test/TEST-*.xml' + annotate_only: true + detailed_summary: true + fail_on_failure: true + + - name: Upload Linux Native Test Reports + uses: actions/upload-artifact@v7 + if: failure() + with: + name: Quartz Linux Native Test Reports + path: quartz/build/reports + test-quartz-ios: # Phase 1 of the iOS support plan # (amethyst/plans/2026-05-24-ios-support.md): keep :quartz green on iOS diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheTest.kt new file mode 100644 index 0000000000..bc61f7aca0 --- /dev/null +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheTest.kt @@ -0,0 +1,319 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.utils.cache + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * Cross-target contract for [LargeCache], the store behind `LocalCache`. + * + * There was no shared suite for this class: the JVM/Android actual is covered only + * indirectly through `LocalCache`, and the linuxX64 and Apple actuals — hand-written + * reimplementations of the same ~40 methods — were covered by nothing at all. This + * runs the same assertions against whichever actual the target picked, so a divergence + * shows up as a test failure instead of at runtime on one platform. + * + * Deliberately order-agnostic: iteration order is insertion order on linux, sorted-key + * order on JVM/Android (`ConcurrentSkipListMap`) and hash order on Apple. Anything + * order-sensitive is compared as a set or sorted first. + */ +class LargeCacheTest { + private fun cacheOf(vararg pairs: Pair) = + LargeCache().apply { + pairs.forEach { put(it.first, it.second) } + } + + @Test + fun emptyCache() { + val cache = LargeCache() + + assertEquals(0, cache.size()) + assertTrue(cache.isEmpty()) + assertNull(cache.get("a")) + assertFalse(cache.containsKey("a")) + assertTrue(cache.keys().isEmpty()) + assertTrue(cache.values().toList().isEmpty()) + } + + @Test + fun putGetAndOverwrite() { + val cache = cacheOf("a" to 1, "b" to 2) + + assertEquals(1, cache.get("a")) + assertEquals(2, cache.get("b")) + assertEquals(2, cache.size()) + assertFalse(cache.isEmpty()) + assertTrue(cache.containsKey("a")) + + cache.put("a", 10) + + assertEquals(10, cache.get("a")) + assertEquals(2, cache.size(), "overwriting a key must not grow the cache") + } + + @Test + fun removeReturnsOldValue() { + val cache = cacheOf("a" to 1, "b" to 2) + + assertEquals(1, cache.remove("a")) + assertNull(cache.get("a")) + assertFalse(cache.containsKey("a")) + assertEquals(1, cache.size()) + + assertNull(cache.remove("a"), "removing an absent key returns null") + assertEquals(1, cache.size()) + } + + @Test + fun clearEmptiesEverything() { + val cache = cacheOf("a" to 1, "b" to 2) + + cache.clear() + + assertEquals(0, cache.size()) + assertTrue(cache.isEmpty()) + assertNull(cache.get("a")) + assertTrue(cache.keys().isEmpty()) + assertTrue(cache.values().toList().isEmpty()) + } + + @Test + fun getOrCreateBuildsOnlyOnce() { + val cache = LargeCache() + var builds = 0 + + assertEquals( + 7, + cache.getOrCreate("k") { + builds++ + 7 + }, + ) + assertEquals( + 7, + cache.getOrCreate("k") { + builds++ + 99 + }, + ) + + assertEquals(1, builds, "the builder must not run for a key that is already present") + assertEquals(7, cache.get("k")) + assertEquals(1, cache.size()) + } + + @Test + fun createIfAbsentReportsWhoInserted() { + val cache = LargeCache() + + assertTrue(cache.createIfAbsent("k") { 1 }, "the first call inserts") + assertFalse(cache.createIfAbsent("k") { 2 }, "the second call must not report an insert") + + assertEquals(1, cache.get("k"), "a losing createIfAbsent must not overwrite") + assertEquals(1, cache.size()) + } + + @Test + fun keysAndValuesSeeLaterWrites() { + val cache = cacheOf("a" to 1) + + // Reads the collections first, so an implementation that caches a snapshot has + // one to go stale. + assertEquals(setOf("a"), cache.keys().toSet()) + assertEquals(listOf(1), cache.values().toList()) + + cache.put("b", 2) + + assertEquals(setOf("a", "b"), cache.keys().toSet()) + assertEquals(listOf(1, 2), cache.values().sorted()) + + cache.remove("a") + + assertEquals(setOf("b"), cache.keys().toSet()) + assertEquals(listOf(2), cache.values().toList()) + } + + @Test + fun bulkReadsSeeLaterWrites() { + val cache = cacheOf("a" to 1) + + // Same idea for the collector path: warm every kind of bulk read, then mutate + // and re-read. Guards the lazily rebuilt snapshot in the linux actual. + assertEquals(1, cache.count { _, _ -> true }) + assertEquals(1, cache.sumOf { _, v -> v }) + + cache.put("b", 2) + assertEquals(2, cache.count { _, _ -> true }) + assertEquals(3, cache.sumOf { _, v -> v }) + + cache.put("a", 10) + assertEquals(12, cache.sumOf { _, v -> v }, "an overwrite must invalidate a cached read view") + + cache.remove("b") + assertEquals(10, cache.sumOf { _, v -> v }) + + cache.getOrCreate("c") { 5 } + assertEquals(15, cache.sumOf { _, v -> v }) + + cache.createIfAbsent("d") { 100 } + assertEquals(115, cache.sumOf { _, v -> v }) + + cache.clear() + assertEquals(0, cache.count { _, _ -> true }) + assertEquals(0, cache.sumOf { _, v -> v }) + } + + @Test + fun forEachVisitsEveryEntry() { + val cache = cacheOf("a" to 1, "b" to 2, "c" to 3) + + val seen = mutableMapOf() + cache.forEach { k, v -> seen[k] = v } + + assertEquals(mapOf("a" to 1, "b" to 2, "c" to 3), seen) + } + + @Test + fun filterAndCount() { + val cache = cacheOf("a" to 1, "b" to 2, "c" to 3, "d" to 4) + + assertEquals(listOf(2, 4), cache.filter { _, v -> v % 2 == 0 }.sorted()) + assertEquals(setOf(2, 4), cache.filterIntoSet { _, v -> v % 2 == 0 }) + assertEquals(2, cache.count { _, v -> v % 2 == 0 }) + assertEquals(1, cache.count { k, _ -> k == "a" }) + assertEquals(0, cache.count { _, _ -> false }) + } + + @Test + fun mapVariants() { + val cache = cacheOf("a" to 1, "b" to 2, "c" to 3) + + assertEquals(listOf(2, 4, 6), cache.map { _, v -> v * 2 }.sorted()) + assertEquals(listOf(2, 3), cache.mapNotNull { _, v -> if (v > 1) v else null }.sorted()) + assertEquals(setOf(2, 3), cache.mapNotNullIntoSet { _, v -> if (v > 1) v else null }) + assertEquals( + listOf(1, 1, 2, 2, 3, 3), + cache.mapFlatten { _, v -> listOf(v, v) }.sorted(), + ) + assertEquals( + setOf(1, 2, 3), + cache.mapFlattenIntoSet { _, v -> listOf(v, v) }, + ) + assertEquals( + listOf(2, 3), + cache.mapFlatten { _, v -> if (v > 1) listOf(v) else null }.sorted(), + "a null collection contributes nothing", + ) + } + + @Test + fun aggregates() { + val cache = cacheOf("a" to 1, "b" to 2, "c" to 3, "d" to 4) + + assertEquals(10, cache.sumOf { _, v -> v }) + assertEquals(10L, cache.sumOfLong { _, v -> v.toLong() }) + assertEquals(4, cache.maxOrNullOf({ _, _ -> true }, naturalOrder())) + assertEquals(3, cache.maxOrNullOf({ _, v -> v % 2 == 1 }, naturalOrder())) + assertNull(cache.maxOrNullOf({ _, _ -> false }, naturalOrder())) + } + + @Test + fun groupings() { + val cache = cacheOf("a" to 1, "b" to 2, "c" to 3, "d" to 4) + + assertEquals( + mapOf(0 to listOf(2, 4), 1 to listOf(1, 3)), + cache.groupBy { _, v -> v % 2 }.mapValues { it.value.sorted() }, + ) + assertEquals(mapOf(0 to 2, 1 to 2), cache.countByGroup { _, v -> v % 2 }) + assertEquals( + mapOf(0 to 6L, 1 to 4L), + cache.sumByGroup({ _, v -> v % 2 }, { _, v -> v.toLong() }), + ) + } + + @Test + fun associates() { + val cache = cacheOf("a" to 1, "b" to 2) + + assertEquals(mapOf(1 to "a", 2 to "b"), cache.associate { k, v -> v to k }) + assertEquals(mapOf("a" to 2, "b" to 4), cache.associateWith { _, v -> v * 2 }) + assertEquals(mapOf("a" to null, "b" to 4), cache.associateWith { _, v -> if (v > 1) v * 2 else null }) + } + + @Test + fun joinToStringRendersEveryEntry() { + val cache = cacheOf("a" to 1, "b" to 2, "c" to 3) + + val rendered = + cache.joinToString( + separator = ",", + prefix = "[", + postfix = "]", + limit = -1, + truncated = "...", + ) { k, v -> "$k=$v" } + + assertTrue(rendered.startsWith("[") && rendered.endsWith("]"), rendered) + assertEquals( + listOf("a=1", "b=2", "c=3"), + rendered + .removePrefix("[") + .removeSuffix("]") + .split(",") + .sorted(), + ) + + assertEquals("[]", LargeCache().joinToString(",", "[", "]", -1, "...") { k, v -> "$k=$v" }) + } + + @Test + fun insertingManyEntriesStaysLinear() { + // Regression guard for the copy-on-write linux actual this replaced, where every + // put rebuilt the whole map: this loop cost ~1.25 billion entry copies there and + // is instant against any O(1)-write implementation. No wall-clock assertion — + // the run time itself is the signal. + val n = 50_000 + val cache = LargeCache() + + for (i in 0 until n) { + cache.put(i, i) + // A full scan every thousandth insert, so an implementation that rebuilds a + // read view on write has to rebuild it ~50 times rather than amortize it away. + if (i % 1_000 == 0) assertEquals(i + 1, cache.count { _, _ -> true }) + } + + assertEquals(n, cache.size()) + assertEquals(0, cache.get(0)) + assertEquals(n - 1, cache.get(n - 1)) + assertEquals(n.toLong() * (n - 1) / 2, cache.sumOfLong { _, v -> v.toLong() }) + + for (i in 0 until n step 2) cache.remove(i) + + assertEquals(n / 2, cache.size()) + assertNull(cache.get(0)) + assertEquals(1, cache.get(1)) + } +} diff --git a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/ConcurrentHashCache.linux.kt b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/ConcurrentHashCache.linux.kt index 29d4d13487..1f697463eb 100644 --- a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/ConcurrentHashCache.linux.kt +++ b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/ConcurrentHashCache.linux.kt @@ -20,30 +20,37 @@ */ package com.vitorpamplona.quartz.utils.cache -import kotlin.concurrent.AtomicReference +import com.vitorpamplona.quartz.utils.concurrent.PlatformLock +import com.vitorpamplona.quartz.utils.concurrent.withLock -// Copy-on-write, mirroring LargeCache.linux: correct and simple; the linux -// target is CI-only so write cost is acceptable. +/** + * Linux/Native actual for [ConcurrentHashCache]. + * + * Was copy-on-write, mirroring the old `LargeCache.linux`: every [put] rebuilt the + * whole map under a CAS retry loop, so writes were O(n) and a decode burst against a + * warm cache was O(n^2). That is a bad shape for this class in particular — its only + * caller, `CachingEventDecoder`, writes once per event arriving from a relay. + * + * Now a plain [HashMap] guarded by a [PlatformLock]: O(1) writes, and no CAS retry + * to livelock under a write burst. Same lock choice and the same residual (a spin + * lock on this target) as `LargeCache.linux.kt` — see its docs. + */ actual class ConcurrentHashCache { - private val mapRef = AtomicReference(HashMap()) + private val lock = PlatformLock() + private val map = HashMap() - actual fun get(key: K): V? = mapRef.value[key] + actual fun get(key: K): V? = lock.withLock { map[key] } actual fun put( key: K, value: V, ) { - while (true) { - val current = mapRef.value - val copy = HashMap(current) - copy[key] = value - if (mapRef.compareAndSet(current, copy)) return - } + lock.withLock { map[key] = value } } - actual fun size(): Int = mapRef.value.size + actual fun size(): Int = lock.withLock { map.size } actual fun clear() { - mapRef.value = HashMap() + lock.withLock { map.clear() } } } diff --git a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCache.linux.kt b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCache.linux.kt index 2e49f22a5c..6887ef1bae 100644 --- a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCache.linux.kt +++ b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCache.linux.kt @@ -20,41 +20,112 @@ */ package com.vitorpamplona.quartz.utils.cache -import kotlin.concurrent.AtomicReference +import com.vitorpamplona.quartz.utils.concurrent.PlatformLock +import com.vitorpamplona.quartz.utils.concurrent.withLock +/** + * Linux/Native actual for [LargeCache] — the store behind Amethyst's `LocalCache`. + * + * ## Why this is not copy-on-write any more + * + * The first cut of this file kept a `LinkedHashMap` inside an `AtomicReference` and + * replaced it wholesale on every write. That made each [put] **O(n) in the size of + * the cache**: inserting n entries cost O(n^2) copies, so a cache holding 100k notes + * paid a 100k-entry map copy per arriving event. It was also *not* actually + * thread-safe — the read-copy-write was not a CAS loop, so two concurrent writers + * silently dropped one of the two writes. + * + * That went unnoticed because no CI job runs the linuxX64 target, so nothing ever + * pushed volume through this class. + * + * ## What it does instead + * + * A single mutable [LinkedHashMap] guarded by a [PlatformLock], plus a lazily built + * read snapshot: + * + * - **Point operations** ([get], [put], [remove], [containsKey], [size], …) take the + * lock, touch the live map, and return. O(1), no copying. + * - **Bulk operations** (`filter`/`map`/`forEach`/…) run against [cachedSnapshot], a + * point-in-time copy rebuilt on the first bulk call after a write and reused until + * the next write. So a copy costs O(n) at most once per write epoch, on operations + * that are already O(n), and a run of reads with no interleaved write copies + * nothing at all. + * + * The snapshot is what lets the bulk operations invoke caller-supplied lambdas + * **outside** the critical section. That matters: `PlatformLock` is not reentrant on + * this target, and `LocalCache` predicates routinely call back into the same cache — + * running them under the lock would self-deadlock. It also removes the + * `ConcurrentModificationException` window the previous `entries.toList()` dance was + * working around. + * + * ## Known residual + * + * Building the snapshot holds the lock for O(n), and the linux `PlatformLock` is a + * spin lock (Kotlin/Native ships no parking lock and there is no Foundation here — + * see `PlatformLock.linux.kt`). A writer racing a snapshot build therefore busy-waits + * for the duration of the copy. That is still strictly better than what it replaces — + * copy-on-write did the same O(n) copy on *every write* and lost concurrent ones — + * but it is the reason `PlatformLock.linux.kt` flags a pthread mutex as the next step + * if this target ever hosts a genuinely contended workload. + * + * Ordering note: iteration follows insertion order here, sorted-key order on + * JVM/Android (`ConcurrentSkipListMap`) and hash order on Apple. Nothing in the + * codebase depends on a specific order, and the `from`/`to` range overloads below + * degrade to a full scan on this target exactly as they did before — they have no + * callers outside the JVM-only `LargeSoftCache`. + */ actual class LargeCache : ICacheOperations { - private val mapRef = AtomicReference(LinkedHashMap()) + private val lock = PlatformLock() - private inline fun withMap(block: (LinkedHashMap) -> R): R = block(mapRef.value) + /** The live store. Every access must hold [lock]. */ + private val map = LinkedHashMap() - private inline fun mutate(block: (LinkedHashMap) -> Unit) { - val copy = LinkedHashMap(mapRef.value) - block(copy) - mapRef.value = copy - } + /** + * A copy of [map] handed to bulk operations so they can run caller lambdas + * without holding [lock]. Null means "stale, rebuild on next use". Guarded by + * [lock]; never mutated once published, so readers may keep it as long as they + * like. + */ + private var cachedSnapshot: Map? = null - actual fun keys(): Set = withMap { LinkedHashSet(it.keys) } - - actual fun values(): Iterable = withMap { ArrayList(it.values) } - - actual fun get(key: K): V? = withMap { it[key] } - - actual fun remove(key: K): V? { - val current = mapRef.value - val value = current[key] - if (value != null) { - mutate { it.remove(key) } + private fun snapshot(): Map = + lock.withLock { + cachedSnapshot ?: LinkedHashMap(map).also { cachedSnapshot = it } } - return value - } - actual fun isEmpty(): Boolean = withMap { it.isEmpty() } + /** Runs [block] over a stable snapshot, outside the lock. */ + private inline fun withMap(block: (Map) -> R): R = block(snapshot()) + + /** Runs [block] over the live map under [lock] without invalidating the snapshot. */ + private inline fun read(block: (MutableMap) -> R): R = lock.withLock { block(map) } + + /** Runs [block] over the live map under [lock] and drops the read snapshot. */ + private inline fun mutate(block: (MutableMap) -> R): R = + lock.withLock { + cachedSnapshot = null + block(map) + } + + actual fun keys(): Set = snapshot().keys + + actual fun values(): Iterable = snapshot().values + + actual fun get(key: K): V? = read { it[key] } + + actual fun remove(key: K): V? = + lock.withLock { + val removed = map.remove(key) + if (removed != null) cachedSnapshot = null + removed + } + + actual fun isEmpty(): Boolean = read { it.isEmpty() } actual fun clear() { - mapRef.value = LinkedHashMap() + mutate { it.clear() } } - actual fun containsKey(key: K): Boolean = withMap { it.containsKey(key) } + actual fun containsKey(key: K): Boolean = read { it.containsKey(key) } actual fun put( key: K, @@ -63,33 +134,61 @@ actual class LargeCache : ICacheOperations { mutate { it[key] = value } } + /** + * Mirrors the JVM actual's `putIfAbsent`: [builder] runs outside the lock (it is + * caller code and must not be able to re-enter a non-reentrant lock), and the + * insert is only published if no one won the race in the meantime. + */ actual fun getOrCreate( key: K, builder: (key: K) -> V, ): V { - val existing = get(key) - if (existing != null) return existing + read { it[key] }?.let { return it } + val newObject = builder(key) - mutate { it[key] = newObject } - return get(key) ?: newObject + + return lock.withLock { + val existing = map[key] + if (existing != null) { + existing + } else { + map[key] = newObject + cachedSnapshot = null + newObject + } + } } + /** + * True only when *this* call inserted the value — matching the JVM actual's + * `putIfAbsent(key, newObject) == null`. The previous implementation returned + * `get(key) != null`, which also reported true when another thread had just + * created the entry, double-firing whatever the caller does with a fresh key. + */ actual fun createIfAbsent( key: K, builder: (key: K) -> V, ): Boolean { - val existing = get(key) - if (existing != null) return false + if (read { it.containsKey(key) }) return false + val newObject = builder(key) - mutate { it[key] = newObject } - return get(key) != null + + return lock.withLock { + if (map.containsKey(key)) { + false + } else { + map[key] = newObject + cachedSnapshot = null + true + } + } } - actual override fun size(): Int = withMap { it.size } + actual override fun size(): Int = read { it.size } actual override fun forEach(consumer: ICacheBiConsumer) { - // Snapshot entries to avoid ConcurrentModificationException - withMap { map -> map.entries.toList() }.forEach { consumer.accept(it.key, it.value) } + // The snapshot is already immutable, so no defensive entries.toList() is needed. + withMap { map -> map.forEach { consumer.accept(it.key, it.value) } } } actual override fun filter(consumer: CacheCollectors.BiFilter): List = withMap { map -> map.filter { consumer.filter(it.key, it.value) }.values.toList() } diff --git a/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheConcurrencyTest.kt b/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheConcurrencyTest.kt new file mode 100644 index 0000000000..07b674700c --- /dev/null +++ b/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheConcurrencyTest.kt @@ -0,0 +1,120 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.utils.cache + +import kotlin.native.concurrent.ObsoleteWorkersApi +import kotlin.native.concurrent.TransferMode +import kotlin.native.concurrent.Worker +import kotlin.test.Test +import kotlin.test.assertEquals + +/** + * The linux actual is the only [LargeCache] whose thread safety is hand-rolled rather + * than delegated to a concurrent map, so it gets its own multi-threaded test. The + * copy-on-write version this replaced would fail every assertion here: its + * read-copy-write was not a CAS loop, so concurrent writers silently dropped each + * other's entries. + * + * Uses `Worker` rather than coroutines on purpose — a coroutine dispatcher gives no + * guarantee of genuine parallelism, and parallelism is the whole point. + */ +@OptIn(ObsoleteWorkersApi::class) +class LargeCacheConcurrencyTest { + private val workerCount = 4 + private val perWorker = 5_000 + + private fun inParallel(job: (workerId: Int) -> R): List { + val workers = List(workerCount) { Worker.start() } + val futures = + workers.mapIndexed { id, worker -> + worker.execute(TransferMode.SAFE, { Pair(job, id) }) { (block, workerId) -> block(workerId) } + } + val results = futures.map { it.result } + workers.forEach { it.requestTermination().result } + return results + } + + @Test + fun concurrentPutsKeepEveryEntry() { + val cache = LargeCache() + + inParallel { id -> + repeat(perWorker) { i -> cache.put(id * perWorker + i, i) } + } + + assertEquals(workerCount * perWorker, cache.size()) + for (id in 0 until workerCount) { + assertEquals(0, cache.get(id * perWorker)) + assertEquals(perWorker - 1, cache.get(id * perWorker + perWorker - 1)) + } + } + + @Test + fun concurrentGetOrCreateBuildsOneValuePerKey() { + val cache = LargeCache() + + // Every worker races for the same 500 keys. Whoever wins, all of them must end + // up holding the identical instance, and the cache must hold exactly 500. + val seen = + inParallel { _ -> + (0 until 500).map { key -> cache.getOrCreate(key) { "v$it" } } + } + + assertEquals(500, cache.size()) + seen.forEach { perWorkerValues -> + perWorkerValues.forEachIndexed { key, value -> + assertEquals(cache.get(key), value, "getOrCreate handed out a value it did not publish") + } + } + } + + @Test + fun concurrentCreateIfAbsentReportsExactlyOneInsertPerKey() { + val cache = LargeCache() + + val insertsPerWorker = inParallel { _ -> (0 until 500).count { key -> cache.createIfAbsent(key) { key } } } + + assertEquals(500, cache.size()) + assertEquals(500, insertsPerWorker.sum(), "exactly one caller per key may report an insert") + } + + @Test + fun bulkReadsStayConsistentWhileWritesLand() { + val cache = LargeCache() + repeat(1_000) { cache.put(it, it) } + + // Writers churn the map while readers walk snapshots of it. A reader must never + // see a torn map, and must never crash on a concurrent modification. + inParallel { id -> + if (id % 2 == 0) { + repeat(perWorker) { i -> cache.put(1_000 + id * perWorker + i, i) } + } else { + repeat(200) { + val sum = cache.sumOfLong { _, v -> v.toLong() } + check(sum >= 0) { "unexpected negative sum $sum" } + cache.count { _, _ -> true } + } + } + } + + assertEquals(1_000 + (workerCount / 2) * perWorker, cache.size()) + } +} diff --git a/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheRangeFallbackTest.kt b/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheRangeFallbackTest.kt new file mode 100644 index 0000000000..1ac079bb92 --- /dev/null +++ b/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheRangeFallbackTest.kt @@ -0,0 +1,73 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.utils.cache + +import kotlin.test.Test +import kotlin.test.assertContentEquals +import kotlin.test.assertEquals + +/** + * The `from`/`to` overloads narrow to a key range only where the backing map is sorted + * (`ConcurrentSkipListMap` on JVM/Android). This target's map is not sorted, and `K` + * carries no `Comparable` bound to sort it by, so every range overload deliberately + * degrades to a full scan and leaves the caller's predicate to do the filtering. + * + * That is safe today because the range overloads have no callers outside the JVM-only + * `LargeSoftCache`. This test pins the behaviour so the fallback stays *consistent* + * with the unbounded form — the property anything sharing code across targets would + * rely on — rather than silently returning a different set on this platform. + * + * Linux-only on purpose: it is a statement about this actual. The Apple actual walks + * its map by index instead, which is a different (and order-dependent) contract. + */ +class LargeCacheRangeFallbackTest { + private val cache = + LargeCache().apply { + put("a", 1) + put("b", 2) + put("c", 3) + } + + private val all = CacheCollectors.BiFilter { _, _ -> true } + + @Test + fun rangeOverloadsMatchTheirUnboundedForm() { + assertContentEquals(cache.filter(all), cache.filter("a", "c", all)) + assertEquals(cache.filterIntoSet(all), cache.filterIntoSet("a", "c", all)) + assertEquals(cache.count(all), cache.count("a", "c", all)) + assertEquals(cache.sumOf { _, v -> v }, cache.sumOf("a", "c") { _, v -> v }) + assertEquals(cache.sumOfLong { _, v -> v.toLong() }, cache.sumOfLong("a", "c") { _, v -> v.toLong() }) + assertContentEquals(cache.map { _, v -> v }, cache.map("a", "c") { _, v -> v }) + assertContentEquals(cache.mapNotNull { _, v -> v }, cache.mapNotNull("a", "c") { _, v -> v }) + assertEquals(cache.associate { k, v -> k to v }, cache.associate("a", "c") { k, v -> k to v }) + assertEquals(cache.associateWith { _, v -> v }, cache.associateWith("a", "c") { _, v -> v }) + assertEquals(cache.countByGroup { _, v -> v % 2 }, cache.countByGroup("a", "c") { _, v -> v % 2 }) + } + + @Test + fun aNarrowerRangeStillScansEverything() { + // Documents the degradation explicitly: on a sorted target this would return + // only "a", here it returns all three. Anyone who later gives this actual real + // range support should update this test rather than discover the difference in + // production. + assertEquals(3, cache.count("a", "a", all)) + } +} diff --git a/quartz/src/nativeMain/kotlin/com/vitorpamplona/quartz/utils/concurrent/ConcurrentMap.native.kt b/quartz/src/nativeMain/kotlin/com/vitorpamplona/quartz/utils/concurrent/ConcurrentMap.native.kt index 8805e8d5a7..4e81970190 100644 --- a/quartz/src/nativeMain/kotlin/com/vitorpamplona/quartz/utils/concurrent/ConcurrentMap.native.kt +++ b/quartz/src/nativeMain/kotlin/com/vitorpamplona/quartz/utils/concurrent/ConcurrentMap.native.kt @@ -23,10 +23,12 @@ package com.vitorpamplona.quartz.utils.concurrent import kotlin.concurrent.atomics.AtomicReference import kotlin.concurrent.atomics.ExperimentalAtomicApi -// Copy-on-write, mirroring ConcurrentHashCache.linux: correct and simple. The -// native targets never run the crawl this backs (it is JVM/Android-only work); -// they only compile it, so the O(n)-per-write cost is irrelevant. A CAS retry -// loop gives getOrPut/merge the same atomicity the JVM actual gets for free. +// Copy-on-write: correct and simple. Unlike LargeCache/ConcurrentHashCache — which +// back LocalCache and the event decoder and were moved off copy-on-write for exactly +// this reason — the native targets never run the crawl this backs (it is +// JVM/Android-only work); they only compile it, so the O(n)-per-write cost is +// irrelevant. A CAS retry loop gives getOrPut/merge the same atomicity the JVM actual +// gets for free. @OptIn(ExperimentalAtomicApi::class) actual class ConcurrentMap { private val ref = AtomicReference(HashMap()) From 65ea34342e06ece3d33468a9cba0bb34729c5f9b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 17:57:22 +0000 Subject: [PATCH 02/18] perf(quartz): make linuxX64's LargeCache lock-free, not lock-based MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the previous commit, which fixed the O(n) write by putting a PlatformLock around a mutable map. That traded one problem for another: the JVM/Android actual is a ConcurrentSkipListMap, where readers never block and writers publish with a CAS, and a global lock is a step down from that — worse, the linux PlatformLock is a spin lock, so a reader could burn a core waiting on a writer that had been descheduled. Keep copy-on-write's shape instead — an immutable map behind an AtomicReference, which is what made reads free in the first place — and fix the two things that were actually wrong with it. Copying a LinkedHashMap is O(n); a HAMT's putting() shares structure and copies only the path to the changed key, O(log32 n). And the read-copy-write was not a CAS loop, so concurrent writers dropped each other's entries; now they retry. Reads (get/containsKey/size/keys/values) are a single atomic load plus a lookup. Bulk operations iterate that same immutable map with no copy, so caller lambdas run outside any critical section and a LocalCache predicate that reaches back into the cache cannot deadlock. Writes are a CAS retry. This is already the house pattern for shared mutable state in commonMain — FilterIndex and nip86 BanStore hold state in one AtomicReference over persistent collections and mutate it with the same loop — and kotlinx-collections-immutable is already a quartz commonMain dependency. Measured on linuxX64 (-opt, ms per loop), vs copy-on-write and vs the lock variant this replaces: n=20,000 fill reads 20 scans mixed copy-on-write 17,949 2 13 25,278 lock+HashMap 1 0 12 35 HAMT+CAS 14 0 19 28 n=200,000 fill reads 20 scans mixed lock+HashMap 44 9 177 6,736 HAMT+CAS 197 12 237 2,486 Write-only, the lock wins ~4x. But LocalCache interleaves full-cache scans with arriving events, and there the lock must rebuild an O(n) read snapshot per write epoch: it loses by 2.7x at 200k. So the non-blocking design also wins the workload that matters. ConcurrentHashCache.linux gets the same treatment; iteration order becomes hash order (as on Apple) rather than insertion order. Nothing depends on it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HxQ1QuyzSkR38iFHbREjoS --- .../utils/cache/ConcurrentHashCache.linux.kt | 37 +-- .../quartz/utils/cache/LargeCache.linux.kt | 230 +++++++++--------- 2 files changed, 141 insertions(+), 126 deletions(-) diff --git a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/ConcurrentHashCache.linux.kt b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/ConcurrentHashCache.linux.kt index 1f697463eb..d59653313b 100644 --- a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/ConcurrentHashCache.linux.kt +++ b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/ConcurrentHashCache.linux.kt @@ -20,37 +20,44 @@ */ package com.vitorpamplona.quartz.utils.cache -import com.vitorpamplona.quartz.utils.concurrent.PlatformLock -import com.vitorpamplona.quartz.utils.concurrent.withLock +import kotlinx.collections.immutable.PersistentMap +import kotlinx.collections.immutable.persistentHashMapOf +import kotlin.concurrent.atomics.AtomicReference +import kotlin.concurrent.atomics.ExperimentalAtomicApi /** * Linux/Native actual for [ConcurrentHashCache]. * - * Was copy-on-write, mirroring the old `LargeCache.linux`: every [put] rebuilt the - * whole map under a CAS retry loop, so writes were O(n) and a decode burst against a - * warm cache was O(n^2). That is a bad shape for this class in particular — its only - * caller, `CachingEventDecoder`, writes once per event arriving from a relay. + * Same fix, and for the same reason, as `LargeCache.linux.kt` — read its docs for the + * full rationale. This was copy-on-write over a plain `HashMap`, so every [put] rebuilt + * the entire map: O(n) per write, and a CAS retry loop that re-did the whole rebuild on + * every lost race. Bad anywhere; worst here, because the only caller is + * `CachingEventDecoder`, which writes once per event arriving from a relay. * - * Now a plain [HashMap] guarded by a [PlatformLock]: O(1) writes, and no CAS retry - * to livelock under a write burst. Same lock choice and the same residual (a spin - * lock on this target) as `LargeCache.linux.kt` — see its docs. + * A HAMT keeps the wait-free single-load read and the non-blocking write while making + * the write O(log32 n) — [PersistentMap.putting] shares structure with the map it came + * from and copies only the path to the changed key. */ +@OptIn(ExperimentalAtomicApi::class) actual class ConcurrentHashCache { - private val lock = PlatformLock() - private val map = HashMap() + private val ref = AtomicReference>(persistentHashMapOf()) - actual fun get(key: K): V? = lock.withLock { map[key] } + actual fun get(key: K): V? = ref.load()[key] actual fun put( key: K, value: V, ) { - lock.withLock { map[key] = value } + while (true) { + val current = ref.load() + val next = current.putting(key, value) + if (next === current || ref.compareAndSet(current, next)) return + } } - actual fun size(): Int = lock.withLock { map.size } + actual fun size(): Int = ref.load().size actual fun clear() { - lock.withLock { map.clear() } + ref.store(persistentHashMapOf()) } } diff --git a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCache.linux.kt b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCache.linux.kt index 6887ef1bae..51251fb309 100644 --- a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCache.linux.kt +++ b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCache.linux.kt @@ -20,174 +20,182 @@ */ package com.vitorpamplona.quartz.utils.cache -import com.vitorpamplona.quartz.utils.concurrent.PlatformLock -import com.vitorpamplona.quartz.utils.concurrent.withLock +import kotlinx.collections.immutable.PersistentMap +import kotlinx.collections.immutable.persistentHashMapOf +import kotlin.concurrent.atomics.AtomicReference +import kotlin.concurrent.atomics.ExperimentalAtomicApi /** * Linux/Native actual for [LargeCache] — the store behind Amethyst's `LocalCache`. * - * ## Why this is not copy-on-write any more + * Mirrors the progress guarantees of the JVM/Android actual (`ConcurrentSkipListMap`): + * **readers never block and never wait for a writer**, and writers publish with a CAS + * rather than by holding a lock. Nothing here can be descheduled while excluding + * everyone else, which is the failure mode `PlatformLock`'s docs describe. * - * The first cut of this file kept a `LinkedHashMap` inside an `AtomicReference` and - * replaced it wholesale on every write. That made each [put] **O(n) in the size of - * the cache**: inserting n entries cost O(n^2) copies, so a cache holding 100k notes - * paid a 100k-entry map copy per arriving event. It was also *not* actually - * thread-safe — the read-copy-write was not a CAS loop, so two concurrent writers - * silently dropped one of the two writes. + * ## Why it is shaped this way * - * That went unnoticed because no CI job runs the linuxX64 target, so nothing ever - * pushed volume through this class. + * The first cut kept a `LinkedHashMap` inside an `AtomicReference` and replaced it + * wholesale on every write. The immutable-snapshot *idea* was right — it is what makes + * reads free — but two things were wrong with it: copying a `LinkedHashMap` makes each + * [put] **O(n) in the size of the cache** (filling n entries costs O(n^2), so a cache + * holding 100k notes paid a 100k-entry copy per arriving event), and the + * read-copy-write was not a CAS loop, so two concurrent writers silently dropped one + * of the two writes. * - * ## What it does instead + * Both fall away by swapping the map for a HAMT. [PersistentMap.putting] shares + * structure with the map it came from and only copies the nodes on the path to the + * changed key — O(log32 n), so ~4 small array copies at a million entries instead of a + * million-entry rehash — and the CAS loop makes concurrent writers retry instead of + * clobbering each other. * - * A single mutable [LinkedHashMap] guarded by a [PlatformLock], plus a lazily built - * read snapshot: + * So: + * - **Reads** ([get], [containsKey], [size], [keys], [values]) are a single atomic load + * plus a lookup on an immutable map. No lock, no allocation, no copy. + * - **Bulk operations** (`filter`/`map`/`forEach`/…) iterate that same immutable map + * directly. No defensive `entries.toList()`, no `ConcurrentModificationException` + * window, and — because no lock is held while a caller's lambda runs — no way for a + * `LocalCache` predicate that reaches back into the cache to deadlock. + * - **Writes** are a CAS retry loop over a structurally shared copy. * - * - **Point operations** ([get], [put], [remove], [containsKey], [size], …) take the - * lock, touch the live map, and return. O(1), no copying. - * - **Bulk operations** (`filter`/`map`/`forEach`/…) run against [cachedSnapshot], a - * point-in-time copy rebuilt on the first bulk call after a write and reused until - * the next write. So a copy costs O(n) at most once per write epoch, on operations - * that are already O(n), and a run of reads with no interleaved write copies - * nothing at all. + * ## What it costs * - * The snapshot is what lets the bulk operations invoke caller-supplied lambdas - * **outside** the critical section. That matters: `PlatformLock` is not reentrant on - * this target, and `LocalCache` predicates routinely call back into the same cache — - * running them under the lock would self-deadlock. It also removes the - * `ConcurrentModificationException` window the previous `entries.toList()` dance was - * working around. + * A write costs more than a `HashMap.put` under a lock would — a few node copies rather + * than one bucket store. Measured on this target (linuxX64, `-opt`, ms for the whole + * loop) against the copy-on-write version this replaces and against a + * `PlatformLock` + `HashMap` variant that was the other candidate: * - * ## Known residual + * ``` + * n=20,000 fill reads 20 scans mixed (put + scan every 1k) + * copy-on-write 17,949 2 13 25,278 + * lock + HashMap 1 0 12 35 + * HAMT + CAS 14 0 19 28 * - * Building the snapshot holds the lock for O(n), and the linux `PlatformLock` is a - * spin lock (Kotlin/Native ships no parking lock and there is no Foundation here — - * see `PlatformLock.linux.kt`). A writer racing a snapshot build therefore busy-waits - * for the duration of the copy. That is still strictly better than what it replaces — - * copy-on-write did the same O(n) copy on *every write* and lost concurrent ones — - * but it is the reason `PlatformLock.linux.kt` flags a pthread mutex as the next step - * if this target ever hosts a genuinely contended workload. + * n=200,000 fill reads 20 scans mixed + * lock + HashMap 44 9 177 6,736 + * HAMT + CAS 197 12 237 2,486 + * ``` * - * Ordering note: iteration follows insertion order here, sorted-key order on - * JVM/Android (`ConcurrentSkipListMap`) and hash order on Apple. Nothing in the - * codebase depends on a specific order, and the `from`/`to` range overloads below - * degrade to a full scan on this target exactly as they did before — they have no - * callers outside the JVM-only `LargeSoftCache`. + * Writing nothing but writes, the lock wins ~4x. But that is not the shape `LocalCache` + * has: it interleaves scans (every feed filters the whole cache) with arriving events, + * and there the lock has to rebuild an O(n) read snapshot after each write epoch, so it + * loses by ~2.7x at 200k. Reads and scans are close either way. The lock-free version + * therefore wins the workload that matters *and* is the one that never blocks a reader. + * + * This is the house pattern for shared mutable state in `commonMain` already — see + * `nip01Core.relay.filters.FilterIndex` and `nip86RelayManagement.server.BanStore`, + * both of which hold their state in one `AtomicReference` over persistent collections + * and mutate it with the same CAS loop. + * + * ## Notes + * + * Anything that reads twice — [remove], [getOrCreate], [createIfAbsent] — retries on a + * lost CAS rather than locking, so the pair is atomic without excluding readers. + * [clear] publishes an empty map unconditionally and can therefore drop a write that + * lands concurrently, exactly as `ConcurrentSkipListMap.clear()` can. + * + * Iteration order is hash order, as on Apple; JVM/Android is sorted-key order + * (`ConcurrentSkipListMap`). Nothing in the codebase depends on a specific order. The + * `from`/`to` range overloads below degrade to a full scan on this target, as they + * always have — they have no callers outside the JVM-only `LargeSoftCache`. */ +@OptIn(ExperimentalAtomicApi::class) actual class LargeCache : ICacheOperations { - private val lock = PlatformLock() - - /** The live store. Every access must hold [lock]. */ - private val map = LinkedHashMap() + private val ref = AtomicReference>(persistentHashMapOf()) /** - * A copy of [map] handed to bulk operations so they can run caller lambdas - * without holding [lock]. Null means "stale, rebuild on next use". Guarded by - * [lock]; never mutated once published, so readers may keep it as long as they - * like. + * Runs [block] against an immutable point-in-time map. Outside any critical + * section, so [block] may call back into this cache freely. */ - private var cachedSnapshot: Map? = null + private inline fun withMap(block: (Map) -> R): R = block(ref.load()) - private fun snapshot(): Map = - lock.withLock { - cachedSnapshot ?: LinkedHashMap(map).also { cachedSnapshot = it } + /** + * Publishes [transform] of the current map with a CAS, retrying if a concurrent + * writer won. [transform] must be pure — it can run more than once — and returning + * the map it was given means "no change", which skips the CAS entirely. + */ + private inline fun mutate(transform: (PersistentMap) -> PersistentMap) { + while (true) { + val current = ref.load() + val next = transform(current) + if (next === current || ref.compareAndSet(current, next)) return } - - /** Runs [block] over a stable snapshot, outside the lock. */ - private inline fun withMap(block: (Map) -> R): R = block(snapshot()) - - /** Runs [block] over the live map under [lock] without invalidating the snapshot. */ - private inline fun read(block: (MutableMap) -> R): R = lock.withLock { block(map) } - - /** Runs [block] over the live map under [lock] and drops the read snapshot. */ - private inline fun mutate(block: (MutableMap) -> R): R = - lock.withLock { - cachedSnapshot = null - block(map) - } - - actual fun keys(): Set = snapshot().keys - - actual fun values(): Iterable = snapshot().values - - actual fun get(key: K): V? = read { it[key] } - - actual fun remove(key: K): V? = - lock.withLock { - val removed = map.remove(key) - if (removed != null) cachedSnapshot = null - removed - } - - actual fun isEmpty(): Boolean = read { it.isEmpty() } - - actual fun clear() { - mutate { it.clear() } } - actual fun containsKey(key: K): Boolean = read { it.containsKey(key) } + actual fun keys(): Set = ref.load().keys + + actual fun values(): Iterable = ref.load().values + + actual fun get(key: K): V? = ref.load()[key] + + actual fun remove(key: K): V? { + while (true) { + val current = ref.load() + val previous = current[key] ?: return null + if (ref.compareAndSet(current, current.removing(key))) return previous + } + } + + actual fun isEmpty(): Boolean = ref.load().isEmpty() + + actual fun clear() { + ref.store(persistentHashMapOf()) + } + + actual fun containsKey(key: K): Boolean = ref.load().containsKey(key) actual fun put( key: K, value: V, ) { - mutate { it[key] = value } + mutate { it.putting(key, value) } } /** - * Mirrors the JVM actual's `putIfAbsent`: [builder] runs outside the lock (it is - * caller code and must not be able to re-enter a non-reentrant lock), and the - * insert is only published if no one won the race in the meantime. + * Mirrors the JVM actual's `putIfAbsent`: [builder] runs at most once — outside the + * retry loop, since it is caller code — and the value is only published if no one + * won the race in the meantime. */ actual fun getOrCreate( key: K, builder: (key: K) -> V, ): V { - read { it[key] }?.let { return it } + ref.load()[key]?.let { return it } val newObject = builder(key) - return lock.withLock { - val existing = map[key] - if (existing != null) { - existing - } else { - map[key] = newObject - cachedSnapshot = null - newObject - } + while (true) { + val current = ref.load() + current[key]?.let { return it } + if (ref.compareAndSet(current, current.putting(key, newObject))) return newObject } } /** * True only when *this* call inserted the value — matching the JVM actual's * `putIfAbsent(key, newObject) == null`. The previous implementation returned - * `get(key) != null`, which also reported true when another thread had just - * created the entry, double-firing whatever the caller does with a fresh key. + * `get(key) != null`, which also reported true when another thread had just created + * the entry, double-firing whatever the caller does with a fresh key. */ actual fun createIfAbsent( key: K, builder: (key: K) -> V, ): Boolean { - if (read { it.containsKey(key) }) return false + if (ref.load().containsKey(key)) return false val newObject = builder(key) - return lock.withLock { - if (map.containsKey(key)) { - false - } else { - map[key] = newObject - cachedSnapshot = null - true - } + while (true) { + val current = ref.load() + if (current.containsKey(key)) return false + if (ref.compareAndSet(current, current.putting(key, newObject))) return true } } - actual override fun size(): Int = read { it.size } + actual override fun size(): Int = ref.load().size actual override fun forEach(consumer: ICacheBiConsumer) { - // The snapshot is already immutable, so no defensive entries.toList() is needed. + // The map is immutable, so this iterates a stable snapshot with no copy. withMap { map -> map.forEach { consumer.accept(it.key, it.value) } } } From 5e662fef0b908f6e33f9d837328588766de4f106 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 18:36:33 +0000 Subject: [PATCH 03/18] perf(quartz): back linuxX64's LargeCache with a striped hash table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The HAMT was the wrong structure for this workload. LocalCache fills on the order of 100,000 entries in a few seconds, and a persistent map allocates a fresh path of ~4-5 nodes for every write — including overwrites, which change no structure at all — then discards it. Over a 100k fill plus scans that is 24 GC cycles. Replace it with a chained hash table: lock-free reads, striped-lock writes. This is ConcurrentHashMap's shape, which Kotlin/Native does not ship. Adding a key prepends one node; overwriting one is a single volatile store into the node already there, allocating nothing; a scan walks the buckets in place. Chain nodes hold `next` immutably so a reader never sees it change, which is what lets reads take no lock at all — structural edits publish a new bucket head, and a resize rebuilds nodes rather than relinking them. Measured on linuxX64 (-opt), 100,000 String keys of event-id length: fill overwrite reads 20 scans mixed GCs heap HAMT + CAS 70 78 6 117 676 24 67MB lock + HashMap 13 6 3 71 1197 36 51MB striped 15 3 3 13 64 1 43MB "mixed" is a full fill with a whole-table scan every 1000 writes — the shape LocalCache actually has. Copy-on-write, the original, is off the scale: 20k entries alone took 18s to fill. Every bulk operation now walks the table directly instead of a snapshot, so scans allocate nothing beyond the result and caller lambdas run outside any critical section — a LocalCache predicate that reaches back into the cache cannot deadlock, and there is no ConcurrentModificationException window. getOrCreate and createIfAbsent are now the JVM actual's bodies verbatim over the same putIfAbsent contract. Honest difference from the JVM actual: ConcurrentSkipListMap is fully non-blocking, whereas writers here block writers hashing to the same one of 16 stripes, for a bucket walk of a few nodes. ConcurrentHashMap makes the same trade. Readers block for nothing. Adds LargeCacheCollisionTest, which forces every key into one bucket so the chain paths — in particular removal, which clones the nodes ahead of the target onto its tail — run deterministically rather than only on a chance collision. ConcurrentHashCache.linux moves onto the same table. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HxQ1QuyzSkR38iFHbREjoS --- .../utils/cache/ConcurrentHashCache.linux.kt | 36 +- .../quartz/utils/cache/LargeCache.linux.kt | 346 ++++++++---------- .../quartz/utils/cache/StripedHashMap.kt | 309 ++++++++++++++++ .../utils/cache/LargeCacheCollisionTest.kt | 140 +++++++ 4 files changed, 605 insertions(+), 226 deletions(-) create mode 100644 quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/StripedHashMap.kt create mode 100644 quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheCollisionTest.kt diff --git a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/ConcurrentHashCache.linux.kt b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/ConcurrentHashCache.linux.kt index d59653313b..294c96f168 100644 --- a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/ConcurrentHashCache.linux.kt +++ b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/ConcurrentHashCache.linux.kt @@ -20,44 +20,30 @@ */ package com.vitorpamplona.quartz.utils.cache -import kotlinx.collections.immutable.PersistentMap -import kotlinx.collections.immutable.persistentHashMapOf -import kotlin.concurrent.atomics.AtomicReference -import kotlin.concurrent.atomics.ExperimentalAtomicApi - /** - * Linux/Native actual for [ConcurrentHashCache]. + * Linux/Native actual for [ConcurrentHashCache], over the same [StripedHashMap] as + * `LargeCache.linux.kt` — read its docs for why. * - * Same fix, and for the same reason, as `LargeCache.linux.kt` — read its docs for the - * full rationale. This was copy-on-write over a plain `HashMap`, so every [put] rebuilt - * the entire map: O(n) per write, and a CAS retry loop that re-did the whole rebuild on - * every lost race. Bad anywhere; worst here, because the only caller is - * `CachingEventDecoder`, which writes once per event arriving from a relay. - * - * A HAMT keeps the wait-free single-load read and the non-blocking write while making - * the write O(log32 n) — [PersistentMap.putting] shares structure with the map it came - * from and copies only the path to the changed key. + * This one was the worst-placed of the copy-on-write caches: its only caller, + * `CachingEventDecoder`, writes once per event arriving from a relay, so every decode + * rebuilt the whole map under a CAS retry loop. Now a write touches one bucket and a + * read takes no lock at all. */ -@OptIn(ExperimentalAtomicApi::class) actual class ConcurrentHashCache { - private val ref = AtomicReference>(persistentHashMapOf()) + private val cache = StripedHashMap() - actual fun get(key: K): V? = ref.load()[key] + actual fun get(key: K): V? = cache.get(key) actual fun put( key: K, value: V, ) { - while (true) { - val current = ref.load() - val next = current.putting(key, value) - if (next === current || ref.compareAndSet(current, next)) return - } + cache.put(key, value) } - actual fun size(): Int = ref.load().size + actual fun size(): Int = cache.size() actual fun clear() { - ref.store(persistentHashMapOf()) + cache.clear() } } diff --git a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCache.linux.kt b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCache.linux.kt index 51251fb309..a718aee614 100644 --- a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCache.linux.kt +++ b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCache.linux.kt @@ -20,160 +20,86 @@ */ package com.vitorpamplona.quartz.utils.cache -import kotlinx.collections.immutable.PersistentMap -import kotlinx.collections.immutable.persistentHashMapOf -import kotlin.concurrent.atomics.AtomicReference -import kotlin.concurrent.atomics.ExperimentalAtomicApi - /** * Linux/Native actual for [LargeCache] — the store behind Amethyst's `LocalCache`. * - * Mirrors the progress guarantees of the JVM/Android actual (`ConcurrentSkipListMap`): - * **readers never block and never wait for a writer**, and writers publish with a CAS - * rather than by holding a lock. Nothing here can be descheduled while excluding - * everyone else, which is the failure mode `PlatformLock`'s docs describe. + * All of the concurrency and the performance rationale lives in [StripedHashMap]; this + * class is only the [ICacheOperations] surface over it. The short version: `LocalCache` + * fills ~100,000 entries in a few seconds while feeds scan the whole cache, so the + * backing store has to take writes at O(1) with next to no garbage *and* let a scan run + * in place without copying. A chained hash table with lock-free reads and striped-lock + * writes — `ConcurrentHashMap`'s shape, which Kotlin/Native does not ship — is the + * structure that does both; copy-on-write and a persistent HAMT each fail one half. * - * ## Why it is shaped this way + * Every bulk operation below walks the table through [StripedHashMap.forEachEntry], + * which takes no lock and allocates nothing beyond the result being built. Two things + * follow, both of which the earlier implementations had to work around: * - * The first cut kept a `LinkedHashMap` inside an `AtomicReference` and replaced it - * wholesale on every write. The immutable-snapshot *idea* was right — it is what makes - * reads free — but two things were wrong with it: copying a `LinkedHashMap` makes each - * [put] **O(n) in the size of the cache** (filling n entries costs O(n^2), so a cache - * holding 100k notes paid a 100k-entry copy per arriving event), and the - * read-copy-write was not a CAS loop, so two concurrent writers silently dropped one - * of the two writes. + * - The caller's lambda never runs inside a critical section, so a `LocalCache` + * predicate that reaches back into the cache cannot deadlock. + * - There is no snapshot and no defensive `entries.toList()`, so no + * `ConcurrentModificationException` window and no per-scan copy. * - * Both fall away by swapping the map for a HAMT. [PersistentMap.putting] shares - * structure with the map it came from and only copies the nodes on the path to the - * changed key — O(log32 n), so ~4 small array copies at a million entries instead of a - * million-entry rehash — and the CAS loop makes concurrent writers retry instead of - * clobbering each other. - * - * So: - * - **Reads** ([get], [containsKey], [size], [keys], [values]) are a single atomic load - * plus a lookup on an immutable map. No lock, no allocation, no copy. - * - **Bulk operations** (`filter`/`map`/`forEach`/…) iterate that same immutable map - * directly. No defensive `entries.toList()`, no `ConcurrentModificationException` - * window, and — because no lock is held while a caller's lambda runs — no way for a - * `LocalCache` predicate that reaches back into the cache to deadlock. - * - **Writes** are a CAS retry loop over a structurally shared copy. - * - * ## What it costs - * - * A write costs more than a `HashMap.put` under a lock would — a few node copies rather - * than one bucket store. Measured on this target (linuxX64, `-opt`, ms for the whole - * loop) against the copy-on-write version this replaces and against a - * `PlatformLock` + `HashMap` variant that was the other candidate: - * - * ``` - * n=20,000 fill reads 20 scans mixed (put + scan every 1k) - * copy-on-write 17,949 2 13 25,278 - * lock + HashMap 1 0 12 35 - * HAMT + CAS 14 0 19 28 - * - * n=200,000 fill reads 20 scans mixed - * lock + HashMap 44 9 177 6,736 - * HAMT + CAS 197 12 237 2,486 - * ``` - * - * Writing nothing but writes, the lock wins ~4x. But that is not the shape `LocalCache` - * has: it interleaves scans (every feed filters the whole cache) with arriving events, - * and there the lock has to rebuild an O(n) read snapshot after each write epoch, so it - * loses by ~2.7x at 200k. Reads and scans are close either way. The lock-free version - * therefore wins the workload that matters *and* is the one that never blocks a reader. - * - * This is the house pattern for shared mutable state in `commonMain` already — see - * `nip01Core.relay.filters.FilterIndex` and `nip86RelayManagement.server.BanStore`, - * both of which hold their state in one `AtomicReference` over persistent collections - * and mutate it with the same CAS loop. - * - * ## Notes - * - * Anything that reads twice — [remove], [getOrCreate], [createIfAbsent] — retries on a - * lost CAS rather than locking, so the pair is atomic without excluding readers. - * [clear] publishes an empty map unconditionally and can therefore drop a write that - * lands concurrently, exactly as `ConcurrentSkipListMap.clear()` can. - * - * Iteration order is hash order, as on Apple; JVM/Android is sorted-key order - * (`ConcurrentSkipListMap`). Nothing in the codebase depends on a specific order. The - * `from`/`to` range overloads below degrade to a full scan on this target, as they - * always have — they have no callers outside the JVM-only `LargeSoftCache`. + * Iteration is weakly consistent and in bucket order. JVM/Android iterates in + * sorted-key order (`ConcurrentSkipListMap`) and Apple in hash order; nothing in the + * codebase depends on a specific one. The `from`/`to` range overloads degrade to a full + * scan here, as they always have — they have no callers outside the JVM-only + * `LargeSoftCache`. */ -@OptIn(ExperimentalAtomicApi::class) actual class LargeCache : ICacheOperations { - private val ref = AtomicReference>(persistentHashMapOf()) + private val cache = StripedHashMap() - /** - * Runs [block] against an immutable point-in-time map. Outside any critical - * section, so [block] may call back into this cache freely. - */ - private inline fun withMap(block: (Map) -> R): R = block(ref.load()) - - /** - * Publishes [transform] of the current map with a CAS, retrying if a concurrent - * writer won. [transform] must be pure — it can run more than once — and returning - * the map it was given means "no change", which skips the CAS entirely. - */ - private inline fun mutate(transform: (PersistentMap) -> PersistentMap) { - while (true) { - val current = ref.load() - val next = transform(current) - if (next === current || ref.compareAndSet(current, next)) return - } + actual fun keys(): Set { + val results = LinkedHashSet(cache.size()) + cache.forEachEntry { key, _ -> results.add(key) } + return results } - actual fun keys(): Set = ref.load().keys - - actual fun values(): Iterable = ref.load().values - - actual fun get(key: K): V? = ref.load()[key] - - actual fun remove(key: K): V? { - while (true) { - val current = ref.load() - val previous = current[key] ?: return null - if (ref.compareAndSet(current, current.removing(key))) return previous - } + actual fun values(): Iterable { + val results = ArrayList(cache.size()) + cache.forEachEntry { _, value -> results.add(value) } + return results } - actual fun isEmpty(): Boolean = ref.load().isEmpty() + actual fun get(key: K): V? = cache.get(key) + + actual fun remove(key: K): V? = cache.remove(key) + + actual fun isEmpty(): Boolean = cache.isEmpty() actual fun clear() { - ref.store(persistentHashMapOf()) + cache.clear() } - actual fun containsKey(key: K): Boolean = ref.load().containsKey(key) + actual fun containsKey(key: K): Boolean = cache.containsKey(key) actual fun put( key: K, value: V, ) { - mutate { it.putting(key, value) } + cache.put(key, value) } - /** - * Mirrors the JVM actual's `putIfAbsent`: [builder] runs at most once — outside the - * retry loop, since it is caller code — and the value is only published if no one - * won the race in the meantime. - */ + // The next two are the JVM actual's bodies verbatim, over the same putIfAbsent + // contract: [builder] runs outside the write path, and the loser of a race keeps + // the winner's value. + actual fun getOrCreate( key: K, builder: (key: K) -> V, ): V { - ref.load()[key]?.let { return it } + val value = cache.get(key) - val newObject = builder(key) - - while (true) { - val current = ref.load() - current[key]?.let { return it } - if (ref.compareAndSet(current, current.putting(key, newObject))) return newObject + return if (value != null) { + value + } else { + val newObject = builder(key) + cache.putIfAbsent(key, newObject) ?: newObject } } /** - * True only when *this* call inserted the value — matching the JVM actual's - * `putIfAbsent(key, newObject) == null`. The previous implementation returned + * True only when *this* call inserted. An early implementation returned * `get(key) != null`, which also reported true when another thread had just created * the entry, double-firing whatever the caller does with a fresh key. */ @@ -181,121 +107,139 @@ actual class LargeCache : ICacheOperations { key: K, builder: (key: K) -> V, ): Boolean { - if (ref.load().containsKey(key)) return false - - val newObject = builder(key) - - while (true) { - val current = ref.load() - if (current.containsKey(key)) return false - if (ref.compareAndSet(current, current.putting(key, newObject))) return true + val value = cache.get(key) + return if (value != null) { + false + } else { + val newObject = builder(key) + cache.putIfAbsent(key, newObject) == null } } - actual override fun size(): Int = ref.load().size + actual override fun size(): Int = cache.size() actual override fun forEach(consumer: ICacheBiConsumer) { - // The map is immutable, so this iterates a stable snapshot with no copy. - withMap { map -> map.forEach { consumer.accept(it.key, it.value) } } + cache.forEachEntry { key, value -> consumer.accept(key, value) } } - actual override fun filter(consumer: CacheCollectors.BiFilter): List = withMap { map -> map.filter { consumer.filter(it.key, it.value) }.values.toList() } + actual override fun filter(consumer: CacheCollectors.BiFilter): List { + val results = ArrayList() + cache.forEachEntry { key, value -> if (consumer.filter(key, value)) results.add(value) } + return results + } - actual override fun filterIntoSet(consumer: CacheCollectors.BiFilter): Set = withMap { map -> map.filter { consumer.filter(it.key, it.value) }.values.toSet() } + actual override fun filterIntoSet(consumer: CacheCollectors.BiFilter): Set { + val results = LinkedHashSet() + cache.forEachEntry { key, value -> if (consumer.filter(key, value)) results.add(value) } + return results + } - actual override fun map(consumer: CacheCollectors.BiNotNullMapper): List = withMap { map -> map.map { consumer.map(it.key, it.value) } } + actual override fun map(consumer: CacheCollectors.BiNotNullMapper): List { + val results = ArrayList(cache.size()) + cache.forEachEntry { key, value -> results.add(consumer.map(key, value)) } + return results + } - actual override fun mapNotNull(consumer: CacheCollectors.BiMapper): List = withMap { map -> map.mapNotNull { consumer.map(it.key, it.value) } } + actual override fun mapNotNull(consumer: CacheCollectors.BiMapper): List { + val results = ArrayList() + cache.forEachEntry { key, value -> consumer.map(key, value)?.let { results.add(it) } } + return results + } - actual override fun mapNotNullIntoSet(consumer: CacheCollectors.BiMapper): Set = mapNotNull(consumer).toSet() + actual override fun mapNotNullIntoSet(consumer: CacheCollectors.BiMapper): Set { + val results = LinkedHashSet() + cache.forEachEntry { key, value -> consumer.map(key, value)?.let { results.add(it) } } + return results + } - actual override fun mapFlatten(consumer: CacheCollectors.BiMapper?>): List = withMap { map -> map.flatMap { entry -> consumer.map(entry.key, entry.value) ?: emptyList() } } + actual override fun mapFlatten(consumer: CacheCollectors.BiMapper?>): List { + val results = ArrayList() + cache.forEachEntry { key, value -> consumer.map(key, value)?.let { results.addAll(it) } } + return results + } - actual override fun mapFlattenIntoSet(consumer: CacheCollectors.BiMapper?>): Set = mapFlatten(consumer).toSet() + actual override fun mapFlattenIntoSet(consumer: CacheCollectors.BiMapper?>): Set { + val results = LinkedHashSet() + cache.forEachEntry { key, value -> consumer.map(key, value)?.let { results.addAll(it) } } + return results + } actual override fun maxOrNullOf( filter: CacheCollectors.BiFilter, comparator: Comparator, - ): V? = - withMap { map -> - var maxV: V? = null - map.forEach { - if (filter.filter(it.key, it.value)) { - if (maxV == null || comparator.compare(it.value, maxV) > 0) { - maxV = it.value - } + ): V? { + var maxV: V? = null + cache.forEachEntry { key, value -> + if (filter.filter(key, value)) { + if (maxV == null || comparator.compare(value, maxV) > 0) { + maxV = value } } - maxV } + return maxV + } - actual override fun sumOf(consumer: CacheCollectors.BiSumOf): Int = - withMap { map -> - var sum = 0 - map.forEach { sum += consumer.map(it.key, it.value) } - sum - } + actual override fun sumOf(consumer: CacheCollectors.BiSumOf): Int { + var sum = 0 + cache.forEachEntry { key, value -> sum += consumer.map(key, value) } + return sum + } - actual override fun sumOfLong(consumer: CacheCollectors.BiSumOfLong): Long = - withMap { map -> - var sum = 0L - map.forEach { sum += consumer.map(it.key, it.value) } - sum - } + actual override fun sumOfLong(consumer: CacheCollectors.BiSumOfLong): Long { + var sum = 0L + cache.forEachEntry { key, value -> sum += consumer.map(key, value) } + return sum + } - actual override fun groupBy(consumer: CacheCollectors.BiNotNullMapper): Map> = - withMap { map -> - val results = HashMap>() - map.forEach { - val group = consumer.map(it.key, it.value) - results.getOrPut(group) { ArrayList() }.add(it.value) - } - results + actual override fun groupBy(consumer: CacheCollectors.BiNotNullMapper): Map> { + val results = HashMap>() + cache.forEachEntry { key, value -> + results.getOrPut(consumer.map(key, value)) { ArrayList() }.add(value) } + return results + } - actual override fun countByGroup(consumer: CacheCollectors.BiNotNullMapper): Map = - withMap { map -> - val results = HashMap() - map.forEach { - val group = consumer.map(it.key, it.value) - results[group] = (results[group] ?: 0) + 1 - } - results + actual override fun countByGroup(consumer: CacheCollectors.BiNotNullMapper): Map { + val results = HashMap() + cache.forEachEntry { key, value -> + val group = consumer.map(key, value) + results[group] = (results[group] ?: 0) + 1 } + return results + } actual override fun sumByGroup( groupMap: CacheCollectors.BiNotNullMapper, sumOf: CacheCollectors.BiNotNullMapper, - ): Map = - withMap { map -> - val results = HashMap() - map.forEach { - val group = groupMap.map(it.key, it.value) - results[group] = (results[group] ?: 0L) + sumOf.map(it.key, it.value) - } - results + ): Map { + val results = HashMap() + cache.forEachEntry { key, value -> + val group = groupMap.map(key, value) + results[group] = (results[group] ?: 0L) + sumOf.map(key, value) } + return results + } - actual override fun count(consumer: CacheCollectors.BiFilter): Int = withMap { map -> map.count { consumer.filter(it.key, it.value) } } + actual override fun count(consumer: CacheCollectors.BiFilter): Int { + var count = 0 + cache.forEachEntry { key, value -> if (consumer.filter(key, value)) count++ } + return count + } - actual override fun associate(transform: (K, V) -> Pair): Map = - withMap { map -> - val results = LinkedHashMap(map.size) - map.forEach { - val pair = transform(it.key, it.value) - results[pair.first] = pair.second - } - results + actual override fun associate(transform: (K, V) -> Pair): Map { + val results = LinkedHashMap(cache.size()) + cache.forEachEntry { key, value -> + val pair = transform(key, value) + results[pair.first] = pair.second } + return results + } - actual override fun associateWith(transform: (K, V) -> U?): Map = - withMap { map -> - val results = LinkedHashMap(map.size) - map.forEach { - results[it.key] = transform(it.key, it.value) - } - results - } + actual override fun associateWith(transform: (K, V) -> U?): Map { + val results = LinkedHashMap(cache.size()) + cache.forEachEntry { key, value -> results[key] = transform(key, value) } + return results + } actual override fun filter( from: K, diff --git a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/StripedHashMap.kt b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/StripedHashMap.kt new file mode 100644 index 0000000000..ec9caba654 --- /dev/null +++ b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/StripedHashMap.kt @@ -0,0 +1,309 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.utils.cache + +import com.vitorpamplona.quartz.utils.concurrent.PlatformLock +import com.vitorpamplona.quartz.utils.concurrent.withLock +import kotlin.concurrent.Volatile +import kotlin.concurrent.atomics.AtomicArray +import kotlin.concurrent.atomics.AtomicInt +import kotlin.concurrent.atomics.ExperimentalAtomicApi + +/** + * A chained hash table with **lock-free reads** and **striped-lock writes** — the shape + * of `java.util.concurrent.ConcurrentHashMap`, which Kotlin/Native has no equivalent of. + * + * Exists because `LocalCache` fills on the order of 100,000 entries in a few seconds, + * and every structure available on this target fails that workload in some way: + * + * - **Copy-on-write over a `HashMap`** (what shipped first) rebuilds the whole map per + * write: O(n) each, O(n^2) to fill. + * - **A persistent HAMT + CAS** is O(log32 n) per write, but allocates a fresh path of + * ~4-5 nodes for *every* write — including overwrites, which change no structure at + * all — and throws the old path away. Measured over a 100k fill plus scans that is 24 + * GC cycles against this table's 1. + * - **One lock around a `HashMap`** writes fast but has to hand bulk operations an O(n) + * copy, because a caller's lambda must not run inside the critical section (the + * linux `PlatformLock` is a spin lock and is not reentrant, and `LocalCache` + * predicates reach back into the cache). + * + * A chained table avoids all three. Structure is only touched when a key is *added* + * (one node, prepended), an overwrite is a single volatile store into the existing + * node, and scans walk the buckets in place with no copy and no lock — so caller + * lambdas run outside any critical section and cannot deadlock. + * + * Measured on linuxX64 (`-opt`), 100,000 String keys of event-id length, ms per phase + * and GC cycles over the whole run: + * + * ``` + * fill overwrite reads 20 scans mixed GCs heap + * copy-on-write* n/a n/a n/a n/a n/a n/a n/a + * HAMT + CAS 70 78 6 117 676 24 67MB + * lock + HashMap 13 6 3 71 1197 36 51MB + * this 15 3 3 13 64 1 43MB + * ``` + * + * (*copy-on-write is off the scale: 20k entries alone took 18s to fill.) "mixed" is a + * full fill with a whole-table scan every 1000 writes, which is the shape `LocalCache` + * actually has — arriving events interleaved with feeds filtering the whole cache. + * Figures are one representative run of several; they were stable to within ~10%, + * except the HAMT's scan column, which wandered between 115ms and 190ms. + * + * ## Concurrency contract + * + * - **Readers never block and never allocate.** [get], [containsKey], [size] and + * [forEachEntry] take no lock. A reader loads [table] once and walks immutable + * `next` links, so it always sees a well-formed chain. + * - **Writers block only against writers hashing to the same stripe**, and only for a + * bucket walk of a few nodes. This is where it differs from the JVM actual's fully + * non-blocking `ConcurrentSkipListMap`; it matches `ConcurrentHashMap`, which also + * locks a bin to write it. + * - Iteration is **weakly consistent**, like both of those: it reflects the table as of + * its first load and may or may not observe writes that land while it runs. It never + * throws, never sees a torn chain, and never needs a defensive copy. + * - A resize takes every stripe lock, so no write can be in flight while it runs. + * Nodes are rebuilt rather than relinked, which is what lets a reader that captured + * the pre-resize table keep walking it safely. + */ +@OptIn(ExperimentalAtomicApi::class) +internal class StripedHashMap { + /** + * [value] is mutable so that overwriting an existing key allocates nothing; [next] + * is not, so that a reader walking a chain can never see it change under them. + * Structural edits publish a new head instead. + */ + internal class Node( + val hash: Int, + val key: K, + @Volatile var value: V, + val next: Node?, + ) + + private val locks = Array(STRIPES) { PlatformLock() } + private val entryCount = AtomicInt(0) + + @Volatile internal var table = AtomicArray?>(INITIAL_CAPACITY) { null } + + @Volatile private var threshold = INITIAL_CAPACITY / 4 * 3 + + /** Spreads the high bits down, so that both the bucket and the stripe see entropy. */ + private fun hashOf(key: K): Int { + val h = key?.hashCode() ?: 0 + return h xor (h ushr 16) + } + + /** + * Derived from the hash alone, never from the table size, so a key keeps the same + * stripe across a resize. + */ + private fun lockFor(hash: Int) = locks[(hash ushr 16) and (STRIPES - 1)] + + fun size(): Int = entryCount.load() + + fun isEmpty(): Boolean = entryCount.load() == 0 + + fun get(key: K): V? { + val hash = hashOf(key) + val current = table + var node = current.loadAt(hash and (current.size - 1)) + while (node != null) { + if (node.hash == hash && node.key == key) return node.value + node = node.next + } + return null + } + + fun containsKey(key: K): Boolean { + val hash = hashOf(key) + val current = table + var node = current.loadAt(hash and (current.size - 1)) + while (node != null) { + if (node.hash == hash && node.key == key) return true + node = node.next + } + return false + } + + fun put( + key: K, + value: V, + ) { + val hash = hashOf(key) + var grew = false + lockFor(hash).withLock { + val current = table + val index = hash and (current.size - 1) + val head = current.loadAt(index) + var node = head + while (node != null) { + if (node.hash == hash && node.key == key) { + // Present already: no structural change, no allocation. + node.value = value + return@withLock + } + node = node.next + } + current.storeAt(index, Node(hash, key, value, head)) + grew = entryCount.fetchAndAdd(1) + 1 > threshold + } + if (grew) growTable() + } + + /** + * Inserts [value] only if [key] is absent, and returns the value already stored — + * or null when this call performed the insert. Exactly `ConcurrentMap.putIfAbsent`, + * which the JVM actual builds `getOrCreate` and `createIfAbsent` out of, including + * its inability to represent a stored null (`ConcurrentSkipListMap` rejects those). + */ + fun putIfAbsent( + key: K, + value: V, + ): V? { + val hash = hashOf(key) + var grew = false + var existing: V? = null + lockFor(hash).withLock { + val current = table + val index = hash and (current.size - 1) + val head = current.loadAt(index) + var node = head + while (node != null) { + if (node.hash == hash && node.key == key) { + existing = node.value + return@withLock + } + node = node.next + } + current.storeAt(index, Node(hash, key, value, head)) + grew = entryCount.fetchAndAdd(1) + 1 > threshold + } + if (grew) growTable() + return existing + } + + fun remove(key: K): V? { + val hash = hashOf(key) + var removed: V? = null + lockFor(hash).withLock { + val current = table + val index = hash and (current.size - 1) + val head = current.loadAt(index) + + var target = head + while (target != null && !(target.hash == hash && target.key == key)) target = target.next + if (target == null) return@withLock + + // `next` is immutable, so the nodes ahead of the removed one are cloned onto + // its tail. A reader still walking the old head sees the entry one last time + // rather than a broken chain. + var rebuilt = target.next + var ahead = head + while (ahead !== target) { + val node = ahead!! + rebuilt = Node(node.hash, node.key, node.value, rebuilt) + ahead = node.next + } + + current.storeAt(index, rebuilt) + entryCount.fetchAndAdd(-1) + removed = target.value + } + return removed + } + + fun clear() { + lockAll() + try { + table = AtomicArray(INITIAL_CAPACITY) { null } + threshold = INITIAL_CAPACITY / 4 * 3 + entryCount.store(0) + } finally { + unlockAll() + } + } + + /** + * Walks every entry without locking. Inline so the caller's body runs with no + * `Function2` dispatch and no captured-variable box per entry, which is what keeps + * a full-cache scan allocation-free. + */ + inline fun forEachEntry(action: (K, V) -> Unit) { + val current = table + for (index in 0 until current.size) { + var node = current.loadAt(index) + while (node != null) { + action(node.key, node.value) + node = node.next + } + } + } + + private fun growTable() { + lockAll() + try { + val old = table + // Another writer may have grown it while this one waited for the locks. + if (entryCount.load() <= threshold) return + if (old.size >= MAX_CAPACITY) { + threshold = Int.MAX_VALUE + return + } + + val capacity = old.size shl 1 + val next = AtomicArray?>(capacity) { null } + for (index in 0 until old.size) { + var node = old.loadAt(index) + while (node != null) { + val target = node.hash and (capacity - 1) + next.storeAt(target, Node(node.hash, node.key, node.value, next.loadAt(target))) + node = node.next + } + } + + table = next + threshold = capacity / 4 * 3 + } finally { + unlockAll() + } + } + + /** Always in index order, and only ever from a thread holding no stripe lock. */ + private fun lockAll() { + for (lock in locks) lock.lock() + } + + private fun unlockAll() { + for (index in locks.indices.reversed()) locks[index].unlock() + } + + companion object { + /** + * Writes are O(1), so a stripe is held for a few nanoseconds and 16 ways is + * plenty — `ConcurrentHashMap` shipped with the same default for years. + */ + private const val STRIPES = 16 + + /** Sized to carry a warm cache's first few thousand entries without a resize. */ + private const val INITIAL_CAPACITY = 1024 + + private const val MAX_CAPACITY = 1 shl 30 + } +} diff --git a/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheCollisionTest.kt b/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheCollisionTest.kt new file mode 100644 index 0000000000..b4ccaeca98 --- /dev/null +++ b/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheCollisionTest.kt @@ -0,0 +1,140 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.utils.cache + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * Drives every key into a single bucket of the [StripedHashMap] backing this target's + * [LargeCache], so the bucket-chain paths run deterministically instead of only when a + * hash happens to collide. + * + * The one that most needs it is removal. Chain nodes hold their `next` immutably — that + * is what lets a reader walk a chain with no lock — so removing from the middle has to + * clone the nodes ahead of the target onto its tail and publish a new head. With + * well-spread keys that path almost never sees a chain longer than two. + */ +class LargeCacheCollisionTest { + /** Every instance lands in the same bucket, and in the same stripe. */ + private data class Collides( + val id: Int, + ) { + override fun hashCode() = 0 + } + + private fun filled(n: Int) = + LargeCache().apply { + for (i in 0 until n) put(Collides(i), i) + } + + @Test + fun readsFindEveryEntryInOneChain() { + val cache = filled(200) + + assertEquals(200, cache.size()) + for (i in 0 until 200) { + assertEquals(i, cache.get(Collides(i)), "entry $i") + assertTrue(cache.containsKey(Collides(i))) + } + assertNull(cache.get(Collides(200))) + assertFalse(cache.containsKey(Collides(200))) + } + + @Test + fun overwriteInAChainReplacesInPlace() { + val cache = filled(200) + + for (i in 0 until 200) cache.put(Collides(i), i * 10) + + assertEquals(200, cache.size(), "overwriting must not lengthen the chain") + for (i in 0 until 200) assertEquals(i * 10, cache.get(Collides(i))) + } + + @Test + fun removeFromTheMiddleKeepsTheRestOfTheChain() { + val cache = filled(200) + + // Head, tail and middle of the chain, in an order that leaves gaps behind. + for (i in 0 until 200 step 3) { + assertEquals(i, cache.remove(Collides(i)), "remove $i returns its value") + } + + val expected = (0 until 200).filter { it % 3 != 0 } + assertEquals(expected.size, cache.size()) + for (i in 0 until 200) { + if (i % 3 == 0) { + assertNull(cache.get(Collides(i)), "entry $i was removed") + } else { + assertEquals(i, cache.get(Collides(i)), "entry $i survived") + } + } + + val seen = mutableListOf() + cache.forEach { _, v -> seen.add(v) } + assertEquals(expected.toSet(), seen.toSet(), "iteration must match the survivors") + assertEquals(expected.size, seen.size, "iteration must not double-count") + + assertNull(cache.remove(Collides(0)), "removing twice is a no-op") + assertEquals(expected.size, cache.size()) + } + + @Test + fun getOrCreateAndCreateIfAbsentWalkTheChain() { + val cache = filled(200) + var builds = 0 + + for (i in 0 until 200) { + assertEquals( + i, + cache.getOrCreate(Collides(i)) { + builds++ + -1 + }, + ) + assertFalse(cache.createIfAbsent(Collides(i)) { -1 }) + } + assertEquals(0, builds, "nothing in the chain should have been rebuilt") + + assertTrue(cache.createIfAbsent(Collides(500)) { 500 }) + assertEquals(500, cache.get(Collides(500))) + assertEquals(201, cache.size()) + } + + @Test + fun clearEmptiesAFullBucket() { + val cache = filled(200) + + cache.clear() + + assertEquals(0, cache.size()) + assertTrue(cache.isEmpty()) + assertNull(cache.get(Collides(7))) + assertEquals(0, cache.count { _, _ -> true }) + + cache.put(Collides(1), 1) + assertEquals(1, cache.size()) + assertEquals(1, cache.get(Collides(1))) + } +} From edab00815d1c01c18bbcdeecef9e240405bf40a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 19:02:55 +0000 Subject: [PATCH 04/18] fix(quartz): route the roles the search extractor was stranding in the body tier Audited all ~70 branches of SearchFieldExtractor.base() against the file's own stated invariant -- "each explicit branch splits exactly the accessors that kind's indexableContent() concatenates" -- for all 133 searchable kinds. Nothing tested it, and it had drifted three ways. The full table is in the PR description; this commit is what it turned up. 19 kinds gain a branch. 1. A title in the wrong tier. 17 kinds fell through to the catch-all, which dumps the whole indexableContent() into the body role, so their titles could never reach the title band a weighted backend gives one. The marketplace family (30017/30018/30019/30020) and the Podcasting 2.0 pair (30054/30055) are the sharpest -- a stall name and an episode title are what people actually type. Kind 9002 is the tell-tale: it edits the very metadata kind 39000 publishes, and 39000 had a branch while 9002 did not. Also branched: 1010, 1065, 1068, 1163, 1985, 2473, 6969, 12473, 38192, 38383. Every kind still falling through is now body-only -- its whole searchable text really is a body (a chat message, a zap comment, a git patch, a DVM prompt) -- so no title is left stranded. 2. A role the branch forgot. hashtags and locations are filled systemically by the tiers() funnel, but websites is per-branch, and four kinds with a public URL were not passing one: GitRepositoryEvent (clones(), the URL most people would search a repo by), MeetingSpaceEvent (endpoint(), the same `streaming` tag kind 30311 already carries), and both nSite kinds (source()). Image, icon and infrastructure URLs stay out on purpose. 3. Drift against indexableContent(). Six kinds concatenated their `t` tags INTO the flat blob while the funnel also carried them as hashtags, so the same words were indexed twice, in the weakest role -- exactly the shape most likely to skew a term-frequency ranker. Fixed by their new branches (1111, 1311, 9002, 30018, 30020, 30054), the same treatment InterestSetEvent and ContactCardEvent already had. Two of those branches avoid creating the same duplication they remove: kind 2473's `alt` is Birdstar's boilerplate wrapper around the two species names, so it is indexed only when commonName() proves it is NOT that shape; kind 12473 is a life LIST, so its unbounded species collection sits in the secondary tier rather than claiming the title band once per bird. Also writes down the PROFILE XOR TIERED contract in the IndexableFields KDoc. The sealed type enforces it, and weighted backends already depend on it: a ranker that scores the two role groups independently and sums them stays correct only while no document can answer from a naming column in each group. A shape filling Profile.name and Tiered.primary at once would claim the top band twice -- measured downstream at ~260 000 against the ~130 000 a whole-field title match earns, i.e. one word per column outranking a document that IS the query. Saying so makes a future both-shapes kind a decision with a known cost rather than an accident. This is derived data: consumers must re-run IEventStore.reindexFullTextSearch() after upgrading. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GznRZiv3zS7V9c2QQ9aMk9 --- .../quartz/nip50Search/IndexableFields.kt | 19 ++ .../nip50Search/SearchFieldExtractor.kt | 161 ++++++++++++- .../nip50Search/SearchFieldExtractorTest.kt | 217 ++++++++++++++++++ 3 files changed, 393 insertions(+), 4 deletions(-) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/IndexableFields.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/IndexableFields.kt index d0e8720804..939914ce33 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/IndexableFields.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/IndexableFields.kt @@ -40,6 +40,25 @@ package com.vitorpamplona.quartz.nip50Search * once values are pre-joined a backend can't unmix them. Values produced by * [SearchFieldExtractor] are trimmed and non-empty; the types themselves do * not enforce it. + * + * ## PROFILE XOR TIERED — a contract, not an implementation detail + * + * A kind fills the profile roles or the content roles, NEVER both. The sealed + * type enforces it today, and weighted backends are entitled to depend on it: + * a ranker that scores the two role groups independently and SUMS them stays + * correct only while no document can answer from a naming column in each + * group. A shape that filled, say, [Profile.name] and [Tiered.primary] at + * once would claim the top band twice — in the store this extractor was + * built for, ~260 000 against the ~130 000 a whole-field title match earns, + * i.e. a document matching one word per column outranking one that IS the + * query. + * + * So a new kind that looks like both is a deliberate decision with a known + * downstream cost, not an accident of the extractor. Pick the shape the kind + * really has and route the rest through it — the way kind 31990 (an app + * handler, whose metadata is a kind-0 clone) returns [Profile] wholesale + * rather than a profile plus a tier. If a future shape genuinely must fill + * both, the backends that sum these groups have to be told. */ sealed interface IndexableFields { fun isEmpty(): Boolean diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractor.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractor.kt index c6ce826f23..b73326c311 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractor.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractor.kt @@ -27,25 +27,39 @@ import com.vitorpamplona.quartz.buzz.teams.TeamEvent import com.vitorpamplona.quartz.buzz.workflow.WorkflowDefEvent import com.vitorpamplona.quartz.experimental.agora.FundraiserEvent import com.vitorpamplona.quartz.experimental.audio.track.AudioTrackEvent +import com.vitorpamplona.quartz.experimental.birdstar.BirdDetectionEvent +import com.vitorpamplona.quartz.experimental.birdstar.BirdexEvent +import com.vitorpamplona.quartz.experimental.edits.TextNoteModificationEvent import com.vitorpamplona.quartz.experimental.fitness.workout.ExerciseTemplateEvent import com.vitorpamplona.quartz.experimental.fitness.workout.WorkoutRecordEvent import com.vitorpamplona.quartz.experimental.interactiveStories.InteractiveStoryBaseEvent import com.vitorpamplona.quartz.experimental.music.playlist.MusicPlaylistEvent import com.vitorpamplona.quartz.experimental.music.track.MusicTrackEvent import com.vitorpamplona.quartz.experimental.nip82SoftwareApps.application.SoftwareApplicationEvent +import com.vitorpamplona.quartz.experimental.nip95.header.FileStorageHeaderEvent import com.vitorpamplona.quartz.experimental.nipsOnNostr.NipTextEvent +import com.vitorpamplona.quartz.experimental.profileGallery.ProfileGalleryEntryEvent +import com.vitorpamplona.quartz.experimental.ps1saves.Ps1SaveEvent import com.vitorpamplona.quartz.experimental.trustedLists.TrustedListEvent +import com.vitorpamplona.quartz.experimental.zapPolls.ZapPollEvent import com.vitorpamplona.quartz.feedDefinition.FeedDefinitionEvent import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.metadata.MetadataEvent import com.vitorpamplona.quartz.nip01Core.tags.hashtags.hashtags import com.vitorpamplona.quartz.nip10Notes.TextNoteEvent import com.vitorpamplona.quartz.nip14Subject.subject +import com.vitorpamplona.quartz.nip15Marketplace.auction.AuctionEvent +import com.vitorpamplona.quartz.nip15Marketplace.marketplace.MarketplaceEvent +import com.vitorpamplona.quartz.nip15Marketplace.product.ProductEvent +import com.vitorpamplona.quartz.nip15Marketplace.stall.StallEvent +import com.vitorpamplona.quartz.nip22Comments.CommentEvent import com.vitorpamplona.quartz.nip23LongContent.LongTextNoteEvent import com.vitorpamplona.quartz.nip28PublicChat.admin.ChannelCreateEvent import com.vitorpamplona.quartz.nip28PublicChat.admin.ChannelMetadataEvent import com.vitorpamplona.quartz.nip29RelayGroups.metadata.GroupMetadataEvent +import com.vitorpamplona.quartz.nip29RelayGroups.moderation.EditMetadataEvent import com.vitorpamplona.quartz.nip30CustomEmoji.pack.EmojiPackEvent +import com.vitorpamplona.quartz.nip32Labeling.LabelEvent import com.vitorpamplona.quartz.nip34Git.issue.GitIssueEvent import com.vitorpamplona.quartz.nip34Git.pr.GitPullRequestEvent import com.vitorpamplona.quartz.nip34Git.repository.GitRepositoryEvent @@ -66,6 +80,7 @@ import com.vitorpamplona.quartz.nip51Lists.videoCurationSet.VideoCurationSetEven import com.vitorpamplona.quartz.nip52Calendar.appt.day.CalendarDateSlotEvent import com.vitorpamplona.quartz.nip52Calendar.appt.time.CalendarTimeSlotEvent import com.vitorpamplona.quartz.nip52Calendar.calendar.CalendarEvent +import com.vitorpamplona.quartz.nip53LiveActivities.chat.LiveActivitiesChatMessageEvent import com.vitorpamplona.quartz.nip53LiveActivities.clip.LiveActivitiesClipEvent import com.vitorpamplona.quartz.nip53LiveActivities.meetingSpaces.MeetingRoomEvent import com.vitorpamplona.quartz.nip53LiveActivities.meetingSpaces.MeetingSpaceEvent @@ -78,6 +93,7 @@ import com.vitorpamplona.quartz.nip5dNapplets.NamedNappletEvent import com.vitorpamplona.quartz.nip5dNapplets.NappletSnapshotEvent import com.vitorpamplona.quartz.nip5dNapplets.RootNappletEvent import com.vitorpamplona.quartz.nip68Picture.PictureEvent +import com.vitorpamplona.quartz.nip69P2pOrderEvents.P2POrderEvent import com.vitorpamplona.quartz.nip71Video.AddressableVideoEvent import com.vitorpamplona.quartz.nip71Video.RegularVideoEvent import com.vitorpamplona.quartz.nip72ModCommunities.definition.CommunityDefinitionEvent @@ -85,6 +101,7 @@ import com.vitorpamplona.quartz.nip75ZapGoals.GoalEvent import com.vitorpamplona.quartz.nip7DThreads.ThreadEvent import com.vitorpamplona.quartz.nip84Highlights.HighlightEvent import com.vitorpamplona.quartz.nip85TrustedAssertions.users.ContactCardEvent +import com.vitorpamplona.quartz.nip88Polls.poll.PollEvent import com.vitorpamplona.quartz.nip89AppHandlers.definition.AppDefinitionEvent import com.vitorpamplona.quartz.nip94FileMetadata.FileHeaderEvent import com.vitorpamplona.quartz.nip99Classifieds.ClassifiedsEvent @@ -92,6 +109,8 @@ import com.vitorpamplona.quartz.nipB0WebBookmarks.WebBookmarkEvent import com.vitorpamplona.quartz.nipC0CodeSnippets.CodeSnippetEvent import com.vitorpamplona.quartz.nipF4Podcasts.episode.PodcastEpisodeEvent import com.vitorpamplona.quartz.nipF4Podcasts.metadata.PodcastMetadataEvent +import com.vitorpamplona.quartz.nipXXPodcasting20.episode.Podcasting20EpisodeEvent +import com.vitorpamplona.quartz.nipXXPodcasting20.trailer.Podcasting20TrailerEvent /** * Decomposes every [SearchableEvent] into [IndexableFields] by priority tier: @@ -148,8 +167,35 @@ object SearchFieldExtractor { tiers(event, event.title(), event.summary(), event.content) } + // NIP-15 kinds 30017/30018/30019/30020 -- the marketplace family + // keeps its name and description inside a JSON `content` blob, so + // the fallback dropped a stall/product/auction NAME into the body + // tier, where a title can never reach the title band. Decoding + // failures still reach the funnel, as they do for the buzz kinds. + is StallEvent -> { + event.stallData()?.let { tiers(event, it.name, it.description, null) } ?: tiers(event, null, null, null) + } + + // ProductEvent.categories() is `t` under another name, so the + // funnel already carries it in the hashtag role -- passing it + // again would index the same words twice. + is ProductEvent -> { + event.productData()?.let { tiers(event, it.name, it.description, null) } ?: tiers(event, null, null, null) + } + + is MarketplaceEvent -> { + event.marketplaceData()?.let { tiers(event, it.name, it.about, null) } ?: tiers(event, null, null, null) + } + + is AuctionEvent -> { + event.auctionData()?.let { tiers(event, it.name, it.description, null) } ?: tiers(event, null, null, null) + } + + // The clone URL is as much a repository's public address as its + // homepage is -- `github.com/owner/repo.git` is how most people + // would search for it -- so both fill the affiliation role. is GitRepositoryEvent -> { - tiers(event, listOf(event.name()), listOf(event.description()), event.content, websites = event.webs()) + tiers(event, listOf(event.name()), listOf(event.description()), event.content, websites = (event.webs() + event.clones()).distinct()) } is GitIssueEvent -> { @@ -238,8 +284,10 @@ object SearchFieldExtractor { tiers(event, event.title(), event.summary(), event.content) } + // endpoint() is the `streaming` tag -- the same role + // LiveActivitiesEvent.streaming() fills above. is MeetingSpaceEvent -> { - tiers(event, event.room(), event.summary(), event.content) + tiers(event, event.room(), event.summary(), event.content, website = event.endpoint()) } is MeetingRoomEvent -> { @@ -282,10 +330,29 @@ object SearchFieldExtractor { tiers(event, listOf(event.title()), listOf(event.description()), null, websites = event.websites()) } + // kinds 30054/30055 -- the Podcasting 2.0 pair carries the same + // title/description shape kind 54 does and was falling through: + // an episode title indexed as body text. topics() is hashtags() + // under another name, so the funnel carries it once. + is Podcasting20EpisodeEvent -> { + tiers(event, event.title(), event.description(), event.content) + } + + is Podcasting20TrailerEvent -> { + tiers(event, event.title(), null, event.content) + } + is GroupMetadataEvent -> { tiers(event, event.name(), event.about(), null) } + // kind 9002 edits the very metadata kind 39000 publishes, so it + // splits the same way -- it was the only half of the pair falling + // through. Its hashtags() is `t`, carried once by the funnel. + is EditMetadataEvent -> { + tiers(event, event.name(), event.about(), null) + } + is InterestSetEvent -> { tiers(event, event.title(), event.description(), null) } @@ -328,12 +395,14 @@ object SearchFieldExtractor { tiers(event, event.title(), event.description(), null, website = event.url()) } + // A site's `source` tag is the URL its files were published + // from -- the same affiliation role a repo's homepage fills. is NamedSiteEvent -> { - tiers(event, event.title(), event.description(), null) + tiers(event, event.title(), event.description(), null, website = event.source()) } is RootSiteEvent -> { - tiers(event, event.title(), event.description(), null) + tiers(event, event.title(), event.description(), null, website = event.source()) } is RootNappletEvent -> { @@ -412,6 +481,50 @@ object SearchFieldExtractor { tiers(event, null, event.summary(), event.content) } + // kinds 1065/1163 -- summary-only kinds. Their whole searchable + // text IS a summary, so it belongs in the summary tier, next to + // kind 1063's, rather than in the body tier the fallback gave it. + is FileStorageHeaderEvent -> { + tiers(event, null, event.summary(), null) + } + + is ProfileGalleryEntryEvent -> { + tiers(event, null, event.summary(), null) + } + + // kind 2473 -- a sighting IS its species, under both names: the + // scientific one from the `n` tag and the vernacular one + // commonName() parses out of the `alt`. Once that parse succeeds + // the alt is only Birdstar's boilerplate wrapper around the two + // ("Bird detection: ()"), so indexing it + // whole would repeat both names in a second role; the alt is + // carried only when it is NOT that shape and so holds text of its + // own. + is BirdDetectionEvent -> { + val common = event.commonName() + tiers(event, listOf(event.speciesName(), common), listOf(event.summary().takeIf { common == null }), null) + } + + // kind 12473 is a life LIST, not a sighting: its species names are + // an unbounded collection, so they sit in the secondary tier with + // a torrent's file names rather than claiming the title band once + // per bird. + is BirdexEvent -> { + tiers(event, emptyList(), listOf(event.summary()) + event.speciesNames(), null) + } + + // kind 38192 -- the save's title is a title; region and filename + // are the keywords beside it, like a torrent's file names. + is Ps1SaveEvent -> { + tiers(event, listOf(event.saveTitle()), listOf(event.summary(), event.region(), event.filename()), null) + } + + // kind 38383 -- an order is looked up by who is offering it; + // currency and payment methods are the keywords that qualify it. + is P2POrderEvent -> { + tiers(event, listOf(event.makerName()), listOf(event.currency()) + event.paymentMethods().orEmpty(), null) + } + is AudioTrackEvent -> { tiers(event, event.subject(), null, null) } @@ -458,6 +571,46 @@ object SearchFieldExtractor { } } + // kind 1010 -- `content` is the proposed replacement text and + // `summary` describes the edit, so they split the way kind 1063's + // summary and body do (indexableContent concatenates them in the + // other order; the roles, not the order, are what a weighted + // backend reads). + is TextNoteModificationEvent -> { + tiers(event, null, event.summary(), event.content) + } + + // kinds 1068/6969 -- the question is the body, the option labels + // are short answer-like values a searcher matches whole, so they + // sit in the secondary tier UNJOINED instead of being appended to + // the body the way indexableContent() has to. + is PollEvent -> { + tiers(event, emptyList(), event.options().map { it.label }, event.content) + } + + is ZapPollEvent -> { + tiers(event, emptyList(), event.pollOptionsArray().map { it.descriptor }, event.content) + } + + // kind 1985 -- the label values are the keywords of the event; + // the reasoning, if any, is the body. + is LabelEvent -> { + tiers(event, emptyList(), event.labels().map { it.label }, event.content) + } + + // kinds 1111/1311 -- body-only kinds whose indexableContent() + // concatenates the `t` tags INTO the body. The funnel already + // carries them in the hashtag role, so passing the body alone + // stops the same words being indexed twice (the ContactCard + // reasoning above, applied to the two chat/comment kinds). + is CommentEvent -> { + tiers(event, null, null, event.content) + } + + is LiveActivitiesChatMessageEvent -> { + tiers(event, null, null, event.content) + } + // kind 1 LAST among the explicit branches, defensively: a future // kind extending the text-note base must hit its own branch first. is TextNoteEvent -> { diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractorTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractorTest.kt index dcebf5b636..227b8b531c 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractorTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractorTest.kt @@ -21,16 +21,31 @@ package com.vitorpamplona.quartz.nip50Search import com.vitorpamplona.quartz.buzz.agentProfiles.AgentProfileEvent +import com.vitorpamplona.quartz.experimental.birdstar.BirdDetectionEvent +import com.vitorpamplona.quartz.experimental.birdstar.BirdexEvent +import com.vitorpamplona.quartz.experimental.nip95.header.FileStorageHeaderEvent +import com.vitorpamplona.quartz.experimental.ps1saves.Ps1SaveEvent import com.vitorpamplona.quartz.experimental.trustedLists.users.UserTrustedListEvent import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.metadata.MetadataEvent import com.vitorpamplona.quartz.nip10Notes.TextNoteEvent +import com.vitorpamplona.quartz.nip15Marketplace.product.ProductEvent +import com.vitorpamplona.quartz.nip15Marketplace.stall.StallEvent import com.vitorpamplona.quartz.nip17Dm.messages.ChatMessageEvent +import com.vitorpamplona.quartz.nip22Comments.CommentEvent import com.vitorpamplona.quartz.nip23LongContent.LongTextNoteEvent +import com.vitorpamplona.quartz.nip29RelayGroups.moderation.EditMetadataEvent +import com.vitorpamplona.quartz.nip32Labeling.LabelEvent +import com.vitorpamplona.quartz.nip34Git.repository.GitRepositoryEvent import com.vitorpamplona.quartz.nip35Torrents.TorrentEvent +import com.vitorpamplona.quartz.nip53LiveActivities.meetingSpaces.MeetingSpaceEvent +import com.vitorpamplona.quartz.nip5aStaticWebsites.NamedSiteEvent +import com.vitorpamplona.quartz.nip69P2pOrderEvents.P2POrderEvent import com.vitorpamplona.quartz.nip85TrustedAssertions.users.ContactCardEvent +import com.vitorpamplona.quartz.nip88Polls.poll.PollEvent import com.vitorpamplona.quartz.nip89AppHandlers.definition.AppDefinitionEvent import com.vitorpamplona.quartz.nipB0WebBookmarks.WebBookmarkEvent +import com.vitorpamplona.quartz.nipXXPodcasting20.episode.Podcasting20EpisodeEvent import kotlin.test.Test import kotlin.test.assertEquals @@ -219,4 +234,206 @@ class SearchFieldExtractorTest { val fields = SearchFieldExtractor.extract(MetadataEvent("9".repeat(64), alice, 1L, tags, "{}", "")) assertEquals(IndexableFields.None, fields) } + + @Test + fun marketplaceNamesReachTheTitleTierNotTheBody() { + // NIP-15 keeps the stall's name inside a JSON content blob. Falling + // through to the catch-all put that NAME in the body tier, where a + // weighted backend can never rank it as a title. + val content = """{"id":"s1","name":"Vitor's Coffee","description":"beans from Minas","currency":"BRL"}""" + val fields = SearchFieldExtractor.extract(StallEvent("20".repeat(32), alice, 1L, arrayOf(arrayOf("d", "s1")), content, "")) + assertEquals(IndexableFields.Tiered(primary = listOf("Vitor's Coffee"), secondary = listOf("beans from Minas")), fields) + } + + @Test + fun productCategoriesAreCarriedOnceAsHashtags() { + // categories() is `t` under another name: indexableContent() + // concatenates it into the flat blob, but the funnel already carries + // it in the hashtag role, so the branch must not pass it again. + val content = """{"id":"p1","stall_id":"s1","name":"Bag of Beans","description":"1kg","currency":"BRL","price":90.0}""" + val tags = arrayOf(arrayOf("d", "p1"), arrayOf("t", "coffee")) + val fields = SearchFieldExtractor.extract(ProductEvent("21".repeat(32), alice, 1L, tags, content, "")) + assertEquals( + IndexableFields.Tiered(primary = listOf("Bag of Beans"), secondary = listOf("1kg"), hashtags = listOf("coffee")), + fields, + ) + } + + @Test + fun unparseableMarketplaceContentStillIndexesItsHashtags() { + val fields = SearchFieldExtractor.extract(StallEvent("22".repeat(32), alice, 1L, arrayOf(arrayOf("t", "coffee")), "not json", "")) + assertEquals(IndexableFields.Tiered(hashtags = listOf("coffee")), fields) + } + + @Test + fun groupMetadataEditsSplitLikeTheMetadataTheyEdit() { + // kind 9002 edits what kind 39000 publishes; it was the only half of + // the pair without a branch. Its hashtags() is `t`: carried once. + val tags = arrayOf(arrayOf("h", "grp"), arrayOf("name", "Nostr Devs"), arrayOf("about", "we build"), arrayOf("t", "nostr")) + val fields = SearchFieldExtractor.extract(EditMetadataEvent("23".repeat(32), alice, 1L, tags, "", "")) + assertEquals( + IndexableFields.Tiered(primary = listOf("Nostr Devs"), secondary = listOf("we build"), hashtags = listOf("nostr")), + fields, + ) + } + + @Test + fun podcasting20EpisodesSplitLikeEveryOtherTitledKind() { + val tags = arrayOf(arrayOf("d", "ep1"), arrayOf("title", "Episode 42"), arrayOf("description", "on search"), arrayOf("t", "podcast")) + val fields = SearchFieldExtractor.extract(Podcasting20EpisodeEvent("24".repeat(32), alice, 1L, tags, "show notes", "")) + assertEquals( + IndexableFields.Tiered(primary = listOf("Episode 42"), secondary = listOf("on search"), text = "show notes", hashtags = listOf("podcast")), + fields, + ) + } + + @Test + fun summaryOnlyKindsUseTheSummaryTier() { + // kind 1065's whole searchable text IS a summary -- it belongs beside + // kind 1063's, not in the body tier. + val tags = arrayOf(arrayOf("summary", "the quarterly report")) + val fields = SearchFieldExtractor.extract(FileStorageHeaderEvent("25".repeat(32), alice, 1L, tags, "", "")) + assertEquals(IndexableFields.Tiered(secondary = listOf("the quarterly report")), fields) + } + + @Test + fun birdSightingsAreNamedByTheirSpeciesUnderBothNames() { + // Birdstar's `alt` is a boilerplate wrapper around the two names, so + // once commonName() has parsed it there is nothing left in it to + // index -- carrying it whole would repeat both names in a second role. + val tags = + arrayOf( + arrayOf("n", "Porphyrio martinica"), + arrayOf("alt", "Bird detection: Purple Gallinule (Porphyrio martinica)"), + ) + val fields = SearchFieldExtractor.extract(BirdDetectionEvent("26".repeat(32), alice, 1L, tags, "", "")) + assertEquals(IndexableFields.Tiered(primary = listOf("Porphyrio martinica", "Purple Gallinule")), fields) + } + + @Test + fun aBirdSightingWithAnUnrecognizedAltStillIndexesIt() { + // A publisher that words the alt differently keeps it: the summary is + // dropped only when commonName() proves it was the boilerplate. + val tags = arrayOf(arrayOf("n", "Ramphastos toco"), arrayOf("alt", "a toucan at the feeder")) + val fields = SearchFieldExtractor.extract(BirdDetectionEvent("3a".repeat(32), alice, 1L, tags, "", "")) + assertEquals( + IndexableFields.Tiered(primary = listOf("Ramphastos toco"), secondary = listOf("a toucan at the feeder")), + fields, + ) + } + + @Test + fun aLifeListKeepsItsSpeciesOutOfTheTitleTier() { + // kind 12473 is an unbounded collection, not a sighting: one title + // band per bird would dilute the tier the way a torrent's file list + // would. + val tags = arrayOf(arrayOf("n", "Ramphastos toco"), arrayOf("n", "Porphyrio martinica"), arrayOf("alt", "my life list")) + val fields = SearchFieldExtractor.extract(BirdexEvent("3b".repeat(32), alice, 1L, tags, "", "")) + assertEquals( + IndexableFields.Tiered(secondary = listOf("my life list", "Ramphastos toco", "Porphyrio martinica")), + fields, + ) + } + + @Test + fun saveTitlesAreTitlesAndTheRestAreKeywords() { + val tags = + arrayOf( + arrayOf("d", "save1"), + arrayOf("title", "Final Fantasy VII"), + arrayOf("filename", "BASCUS-94163"), + arrayOf("region", "NTSC-U"), + arrayOf("alt", "a memory card save"), + ) + val fields = SearchFieldExtractor.extract(Ps1SaveEvent("27".repeat(32), alice, 1L, tags, "", "")) + assertEquals( + IndexableFields.Tiered( + primary = listOf("Final Fantasy VII"), + secondary = listOf("a memory card save", "NTSC-U", "BASCUS-94163"), + ), + fields, + ) + } + + @Test + fun p2pOrdersAreNamedByTheirMaker() { + val tags = arrayOf(arrayOf("d", "o1"), arrayOf("name", "Satoshi"), arrayOf("f", "BRL"), arrayOf("pm", "pix", "wire")) + val fields = SearchFieldExtractor.extract(P2POrderEvent("28".repeat(32), alice, 1L, tags, "", "")) + assertEquals( + IndexableFields.Tiered(primary = listOf("Satoshi"), secondary = listOf("BRL", "pix", "wire")), + fields, + ) + } + + @Test + fun pollOptionsAreCarriedUnjoinedInTheSecondaryTier() { + // indexableContent() has to append the labels to the body; the roles + // keep them separate, so the backend chooses how to weight them. + val tags = arrayOf(arrayOf("option", "1", "Coffee"), arrayOf("option", "2", "Tea")) + val fields = SearchFieldExtractor.extract(PollEvent("29".repeat(32), alice, 1L, tags, "what should I drink?", "")) + assertEquals( + IndexableFields.Tiered(secondary = listOf("Coffee", "Tea"), text = "what should I drink?"), + fields, + ) + } + + @Test + fun labelValuesAreKeywordsNotBody() { + val tags = arrayOf(arrayOf("l", "spam", "report"), arrayOf("L", "report")) + val fields = SearchFieldExtractor.extract(LabelEvent("2b".repeat(32), alice, 1L, tags, "obvious bot", "")) + assertEquals(IndexableFields.Tiered(secondary = listOf("spam"), text = "obvious bot"), fields) + } + + @Test + fun commentHashtagsAreNotIndexedTwice() { + // indexableContent() concatenates the `t` tags INTO the body for kind + // 1111; the funnel already carries them in the hashtag role, so the + // branch passes the body alone. + val tags = arrayOf(arrayOf("t", "nostr")) + val fields = SearchFieldExtractor.extract(CommentEvent("2c".repeat(32), alice, 1L, tags, "good point", "")) + assertEquals(IndexableFields.Tiered(text = "good point", hashtags = listOf("nostr")), fields) + } + + @Test + fun repositoriesAreFindableByCloneUrlAndHomepage() { + val tags = + arrayOf( + arrayOf("d", "amethyst"), + arrayOf("name", "Amethyst"), + arrayOf("description", "a nostr client"), + arrayOf("web", "https://amethyst.social"), + arrayOf("clone", "https://github.com/vitorpamplona/amethyst.git"), + // A repo whose homepage IS its clone URL must not index it twice. + arrayOf("clone", "https://amethyst.social"), + ) + val fields = SearchFieldExtractor.extract(GitRepositoryEvent("2d".repeat(32), alice, 1L, tags, "", "")) + assertEquals( + IndexableFields.Tiered( + primary = listOf("Amethyst"), + secondary = listOf("a nostr client"), + websites = listOf("https://amethyst.social", "https://github.com/vitorpamplona/amethyst.git"), + ), + fields, + ) + } + + @Test + fun meetingSpacesCarryTheirStreamingUrlLikeLiveActivitiesDo() { + val tags = arrayOf(arrayOf("d", "room1"), arrayOf("title", "Design Sync"), arrayOf("streaming", "https://nests.example/room1")) + val fields = SearchFieldExtractor.extract(MeetingSpaceEvent("2e".repeat(32), alice, 1L, tags, "", "")) + assertEquals( + IndexableFields.Tiered(primary = listOf("Design Sync"), websites = listOf("https://nests.example/room1")), + fields, + ) + } + + @Test + fun staticSitesCarryTheirSourceUrl() { + val tags = arrayOf(arrayOf("d", "blog"), arrayOf("title", "My Blog"), arrayOf("source", "https://github.com/me/blog")) + val fields = SearchFieldExtractor.extract(NamedSiteEvent("2f".repeat(32), alice, 1L, tags, "", "")) + assertEquals( + IndexableFields.Tiered(primary = listOf("My Blog"), websites = listOf("https://github.com/me/blog")), + fields, + ) + } } From d759741f9641c7909eb186a1201d17fce2bc5738 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 19:06:53 +0000 Subject: [PATCH 05/18] fix(quartz): make the whole linuxX64 test suite pass; widen the CI leg MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 78 failures on this target were not 78 unimplemented actuals. Two root causes accounted for all of them. TestResourceLoader.linux was a TODO(), so every vector-driven suite failed before it reached any production code: the full MLS interop set, NIP-44, the NIP-01 hint indexer, the SQLite store's large-DB tests and the Bolt12 payer proofs — 69 tests. Implemented over platform.posix (linuxX64 has no Foundation for the Apple actual's NSData path), resolving against the same TEST_RESOURCES_ROOT that build.gradle.kts already exports onto every KotlinNativeTest task. The read is one ftell-sized allocation filled by fread, so a vector file costs exactly one ByteArray — less than the JVM actual's bufferedReader().readText(), which grows a StringBuilder as it goes. UriParser.linux never URL-decoded query values or fragments, though the JVM actual runs both through URLDecoder.decode(.., "UTF-8"). Every NIP-47 failure was one symptom of that: relay=wss%3A%2F%2Frelay.damus.io reached RelayUrlNormalizer still percent-encoded and came back "Invalid relay Url" (6 tests), and the deep-link round trips compared an encoded string against a plain one (3 tests). Added a decoder matching URLDecoder where the behaviour is observable — '+' to space, a run of consecutive %XX decoded as one UTF-8 sequence, malformed escapes throwing IllegalArgumentException — with the same short-circuit URLDecoder makes, returning the original instance when there is nothing to decode. Two other divergences fixed while there: getQueryParameter returned an empty list where the JVM returns null for an absent parameter, and the query string was re-split on every call rather than parsed once into a lazy map, so a URI read for four parameters was parsed four times. With those, linuxX64Test is 3495 tests, 0 failures, so the CI leg added alongside the LargeCache work drops its cache-package filter and runs the whole :quartz suite. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HxQ1QuyzSkR38iFHbREjoS --- .github/workflows/build.yml | 32 ++--- .../quartz/utils/UriParser.linux.kt | 134 +++++++++++++++--- .../quartz/TestResourceLoader.linux.kt | 82 ++++++++++- 3 files changed, 197 insertions(+), 51 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index c130551bce..7c55e0819b 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -194,24 +194,15 @@ jobs: name: geode Test Reports path: geode/build/reports - # linuxX64 is the only target whose LargeCache / ConcurrentHashCache actuals are - # hand-written concurrent maps rather than a delegation to a platform concurrent - # collection, and until this job existed nothing ran them: the target was compiled - # by no CI leg at all. That is how a copy-on-write LargeCache with O(n) writes and a - # non-atomic read-copy-write (concurrent writers silently dropped entries) sat in - # the tree unnoticed. + # Until this job existed nothing ran the linuxX64 target at all — it was compiled by + # no CI leg. That is how a copy-on-write LargeCache with O(n) writes and a non-atomic + # read-copy-write (concurrent writers silently dropped entries) sat in the tree + # unnoticed, and how TestResourceLoader stayed a TODO() that failed every vector-driven + # suite on the target. # - # Scoped to the cache/concurrency packages on purpose. The full linuxX64Test suite - # is 3,490 tests with 78 pre-existing failures, essentially all of them `TODO()` - # stubs in linux actuals that were never written — MLS crypto, the SQLite driver, - # NIP-44, Bolt12 — plus two URL-handling divergences. Filling those in is its own - # project; gating PRs on them today would just mean a permanently red job. The - # filter keeps the leg meaningful and green, and widening it is a one-line change - # once the native actuals land. - # - # This still compiles and links the whole module for linuxX64, so a commonMain or - # commonTest source that reaches for a JVM-only API fails here too — on a target - # with no Foundation to fall back on the way Apple has. + # Runs the whole :quartz suite on a Linux Native frontend, which also catches a + # commonMain or commonTest source reaching for a JVM-only API on a target that, unlike + # Apple, has no Foundation to fall back on. test-quartz-linux-native: needs: lint runs-on: ubuntu-latest @@ -242,11 +233,8 @@ jobs: key: konan-${{ runner.os }}-${{ hashFiles('gradle/libs.versions.toml') }} restore-keys: konan-${{ runner.os }}- - - name: Test Quartz caches on Linux Native - run: | - ./gradlew :quartz:linuxX64Test \ - --tests "com.vitorpamplona.quartz.utils.cache.*" \ - --tests "com.vitorpamplona.quartz.utils.concurrent.*" + - name: Test Quartz on Linux Native + run: ./gradlew :quartz:linuxX64Test - name: Linux Native Test Report uses: mikepenz/action-junit-report@a9170d5795813c01ab4901ffb045b52bab4ab09d # v6.5.0 diff --git a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/UriParser.linux.kt b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/UriParser.linux.kt index 5f5d9f0c27..448f4b8c8f 100644 --- a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/UriParser.linux.kt +++ b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/UriParser.linux.kt @@ -121,41 +121,129 @@ actual class UriParser actual constructor( actual fun path(): String? = parsedPath - actual fun queryParameterNames(): Set { - val query = parsedQuery ?: return emptySet() - return query - .split('&') - .map { param -> - val eqIndex = param.indexOf('=') - if (eqIndex >= 0) param.substring(0, eqIndex) else param - }.toSet() - } + /** + * Parsed once and reused, mirroring the JVM actual's lazy map. The previous version + * re-split the entire query string on every [getQueryParameter] call, so a URI read + * for four parameters was parsed four times. + */ + private val queryParameters: Map> by lazy { + parsedQuery?.ifBlank { null }?.let { query -> + val params = mutableMapOf>() - actual fun getQueryParameter(param: String): List? { - val query = parsedQuery ?: return null - return query - .split('&') - .filter { part -> - val eqIndex = part.indexOf('=') - if (eqIndex >= 0) part.substring(0, eqIndex) == param else part == param - }.map { part -> - val eqIndex = part.indexOf('=') - if (eqIndex >= 0) part.substring(eqIndex + 1) else "" + query.split('&').forEach { paramValue -> + val parts = paramValue.split("=", limit = 2) + val currentValue = + params.getOrPut(parts[0]) { + mutableListOf() + } + + if (parts.size == 2) { + currentValue.add(percentDecode(parts[1])) + } else { + currentValue.add("") + } } + + params + } ?: emptyMap() } - val fragments: Map by lazy { + private val parsedFragments: Map by lazy { parsedFragment?.ifBlank { null }?.let { keyValuePair -> keyValuePair.split('&').associate { paramValue -> val parts = paramValue.split("=", limit = 2) if (parts.size == 2) { - parts[0] to parts[1] + parts[0] to percentDecode(parts[1]) } else { - parts[0] to "" + parts[0] to "" // Handle parameters without a value } } } ?: emptyMap() } - actual fun fragments(): Map = fragments + actual fun queryParameterNames(): Set = queryParameters.keys + + /** Null — not an empty list — when the parameter is absent, as on the JVM. */ + actual fun getQueryParameter(param: String): List? = queryParameters[param] + + actual fun fragments(): Map = parsedFragments } + +/** + * `java.net.URLDecoder.decode(value, "UTF-8")` — which is literally what the JVM actual + * calls — for a target with no `java.net`. + * + * This is the whole reason NIP-47 failed on linuxX64: the parser returned query values + * exactly as they appeared in the URI, so `relay=wss%3A%2F%2Frelay.damus.io` reached + * `RelayUrlNormalizer` still percent-encoded and came back "Invalid relay Url". Decoding + * belongs here rather than in each caller, because the JVM and Apple actuals both hand + * back decoded values and common code is written against that. + * + * Matches `URLDecoder` in the details that are observable: `+` becomes a space, a run of + * consecutive `%XX` is decoded as one UTF-8 sequence (so multi-byte characters survive), + * every other character passes through, and a malformed escape throws + * [IllegalArgumentException] rather than being silently kept — the same failure the JVM + * gives for the same input. + */ +private fun percentDecode(value: String): String { + // The short-circuit URLDecoder also makes: with nothing to change, return the + // original instance rather than rebuilding it. Most query values hit this. + if (value.indexOf('%') < 0 && value.indexOf('+') < 0) return value + + val result = StringBuilder(value.length) + var index = 0 + // Sized on first use for the longest run that could still follow, then reused — + // one allocation for the whole string, as in URLDecoder. + var escaped: ByteArray? = null + + while (index < value.length) { + when (val char = value[index]) { + '+' -> { + result.append(' ') + index++ + } + + '%' -> { + val buffer = escaped ?: ByteArray((value.length - index) / 3).also { escaped = it } + var count = 0 + while (index + 2 < value.length && value[index] == '%') { + buffer[count++] = decodeEscape(value, index) + index += 3 + } + if (index < value.length && value[index] == '%') { + throw IllegalArgumentException("URLDecoder: Incomplete trailing escape (%) pattern") + } + result.append(buffer.decodeToString(0, count)) + } + + else -> { + result.append(char) + index++ + } + } + } + + return result.toString() +} + +private fun decodeEscape( + value: String, + index: Int, +): Byte { + val high = hexDigit(value[index + 1]) + val low = hexDigit(value[index + 2]) + if (high < 0 || low < 0) { + throw IllegalArgumentException( + "URLDecoder: Illegal hex characters in escape (%) pattern - ${value.substring(index, index + 3)}", + ) + } + return ((high shl 4) or low).toByte() +} + +private fun hexDigit(char: Char): Int = + when (char) { + in '0'..'9' -> char - '0' + in 'a'..'f' -> char - 'a' + 10 + in 'A'..'F' -> char - 'A' + 10 + else -> -1 + } diff --git a/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/TestResourceLoader.linux.kt b/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/TestResourceLoader.linux.kt index 3997dd8575..3d8f191d19 100644 --- a/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/TestResourceLoader.linux.kt +++ b/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/TestResourceLoader.linux.kt @@ -20,12 +20,82 @@ */ package com.vitorpamplona.quartz -actual class TestResourceLoader actual constructor() { - actual fun loadDecompressString(file: String): String { - TODO("Not yet implemented") - } +import com.vitorpamplona.quartz.utils.GZip +import kotlinx.cinterop.ExperimentalForeignApi +import kotlinx.cinterop.addressOf +import kotlinx.cinterop.convert +import kotlinx.cinterop.toKString +import kotlinx.cinterop.usePinned +import platform.posix.SEEK_END +import platform.posix.SEEK_SET +import platform.posix.fclose +import platform.posix.fopen +import platform.posix.fread +import platform.posix.fseek +import platform.posix.ftell +import platform.posix.getenv - actual fun loadString(file: String): String { - TODO("Not yet implemented") +/** + * Linux/Native actual for [TestResourceLoader]. + * + * Until this existed it was `TODO()`, which failed 69 tests on this target — every + * suite driven by a vector file: the whole MLS interop set, NIP-44, the NIP-01 hint + * indexer, the SQLite store's large-DB tests and the Bolt12 payer proofs. None of them + * were failing because of missing production code; they could not read their input. + * + * Resolves paths against `TEST_RESOURCES_ROOT`, the same environment variable the Apple + * actual uses, exported onto every `KotlinNativeTest` task by `quartz/build.gradle.kts`. + * Reads through `platform.posix` rather than Foundation, which linuxX64 does not have. + * + * The read is a single `stat`-sized allocation filled by `fread`, so a vector file + * costs exactly one `ByteArray` — less than the JVM actual's `bufferedReader().readText()`, + * which grows a `StringBuilder` as it goes. + */ +@OptIn(ExperimentalForeignApi::class) +actual class TestResourceLoader actual constructor() { + actual fun loadDecompressString(file: String): String = GZip.decompress(readBytes(file)) + + actual fun loadString(file: String): String = readBytes(file).decodeToString() + + private fun readBytes(file: String): ByteArray { + val root = + getenv("TEST_RESOURCES_ROOT")?.toKString() + ?: throw IllegalStateException( + "TEST_RESOURCES_ROOT is not set. quartz/build.gradle.kts exports it onto every " + + "KotlinNativeTest task; running the test binary directly has to set it too.", + ) + + val path = "$root/$file" + val handle = fopen(path, "rb") ?: throw IllegalArgumentException("Resource not found: $path") + + try { + if (fseek(handle, 0, SEEK_END) != 0) throw IllegalArgumentException("Resource is not seekable: $path") + val size = ftell(handle) + if (size < 0L) throw IllegalArgumentException("Cannot determine the size of: $path") + if (size == 0L) return ByteArray(0) + if (fseek(handle, 0, SEEK_SET) != 0) throw IllegalArgumentException("Cannot rewind: $path") + + val bytes = ByteArray(size.toInt()) + bytes.usePinned { pinned -> + var read = 0 + while (read < bytes.size) { + val count = + fread( + pinned.addressOf(read), + 1.convert(), + (bytes.size - read).convert(), + handle, + ).toInt() + if (count <= 0) break + read += count + } + if (read != bytes.size) { + throw IllegalArgumentException("Short read on $path: got $read of ${bytes.size} bytes") + } + } + return bytes + } finally { + fclose(handle) + } } } From c52fbc428eb6e6fcb5cf4b6bf2d48fd5e6a6713f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 19:29:38 +0000 Subject: [PATCH 06/18] fix(quartz): stop dropping a bird sighting's alt text, and stop allocating per role Two findings from an audit of this PR's own diff. BUG. The kind 2473 branch dropped the `alt` tag whenever commonName() parsed one out of it, on the reasoning that the alt is only Birdstar's boilerplate wrapper around the two species names. But commonName() matches a PREFIX and then cuts at the last " (", so a publisher can write anything after the parenthetical and it parses just the same: "Bird detection: Purple Gallinule (Porphyrio martinica) at Lake Merritt, 7am" yielded "Purple Gallinule" and the tail reached NO role, while indexableContent() still carried it. That is exactly the drift against the flat form this PR exists to remove, introduced by the PR itself. The alt now always reaches the summary tier: the duplicate it repeats there lands in the weakest role, whereas the drop cost recall outright. PERFORMANCE. Extraction runs once per stored event and per full reindex, and the funnel allocated a throwaway list per role whether or not the role had anything in it. The single-value tiers() overload wrapped each of its three values in a list only for cleanAll() to build another; cleanAll() allocated even when every value was null; the hashtag role called hashtags(), which allocates unconditionally, on every event including the great majority carrying no `t` tag; and locationValues() allocated a list per event to hold, almost always, nothing. Both overloads now end in one build() -- so hashtags and locations are still filled in a single place no branch can forget -- and each collector allocates lazily. Measured with getThreadAllocatedBytes over 1M extractions, JIT-warm: kind 1, no tags 160 -> 40 B/event kind 1, six tags 528 -> 168 B/event kind 30023 title+summary 272 -> 88 B/event The hash of every extracted value is unchanged across the A/B, and the guard added before hashtags() is HashtagTag.parse's own acceptance test, so it cannot skip a tag the accessor would have returned. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GznRZiv3zS7V9c2QQ9aMk9 --- .../nip50Search/SearchFieldExtractor.kt | 98 ++++++++++++++----- .../nip50Search/SearchFieldExtractorTest.kt | 42 ++++++-- 2 files changed, 111 insertions(+), 29 deletions(-) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractor.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractor.kt index b73326c311..97fcb8b7cb 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractor.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractor.kt @@ -44,7 +44,10 @@ import com.vitorpamplona.quartz.experimental.trustedLists.TrustedListEvent import com.vitorpamplona.quartz.experimental.zapPolls.ZapPollEvent import com.vitorpamplona.quartz.feedDefinition.FeedDefinitionEvent import com.vitorpamplona.quartz.nip01Core.core.Event +import com.vitorpamplona.quartz.nip01Core.core.fastAny +import com.vitorpamplona.quartz.nip01Core.core.fastForEach import com.vitorpamplona.quartz.nip01Core.metadata.MetadataEvent +import com.vitorpamplona.quartz.nip01Core.tags.hashtags.HashtagTag import com.vitorpamplona.quartz.nip01Core.tags.hashtags.hashtags import com.vitorpamplona.quartz.nip10Notes.TextNoteEvent import com.vitorpamplona.quartz.nip14Subject.subject @@ -494,15 +497,18 @@ object SearchFieldExtractor { // kind 2473 -- a sighting IS its species, under both names: the // scientific one from the `n` tag and the vernacular one - // commonName() parses out of the `alt`. Once that parse succeeds - // the alt is only Birdstar's boilerplate wrapper around the two - // ("Bird detection: ()"), so indexing it - // whole would repeat both names in a second role; the alt is - // carried only when it is NOT that shape and so holds text of its - // own. + // commonName() parses out of the `alt`. The `alt` itself still + // goes to the summary tier whole. It is usually just Birdstar's + // boilerplate wrapper around those two names ("Bird detection: + // ()"), so this repeats them in a second + // role -- but only commonName()'s PREFIX match decides that the + // alt is boilerplate, and a publisher can write anything after + // the parenthetical. Dropping the alt on a prefix match lost that + // tail from every role, which is precisely the drift against + // indexableContent() this file exists to prevent: the repeat + // costs a duplicate in the weakest role, the drop cost recall. is BirdDetectionEvent -> { - val common = event.commonName() - tiers(event, listOf(event.speciesName(), common), listOf(event.summary().takeIf { common == null }), null) + tiers(event, listOf(event.speciesName(), event.commonName()), listOf(event.summary()), null) } // kind 12473 is a life LIST, not a sighting: its species names are @@ -628,46 +634,94 @@ object SearchFieldExtractor { } } - /** Single-value convenience over the list funnel — most kinds carry one title, one summary, one body. */ + /** + * Single-value convenience over the list funnel — most kinds carry one + * title, one summary, one body. Cleans each value straight into its role + * rather than wrapping it in a list first: extraction runs once per stored + * event, and the wrappers were three throwaway lists on every one of them. + */ private fun tiers( event: Event, primary: String?, secondary: String?, text: String?, website: String? = null, - ) = tiers(event, listOf(primary), listOf(secondary), text, listOf(website)) + ) = build(event, cleanOne(primary), cleanOne(secondary), text, cleanOne(website)) - /** - * The one funnel every content branch uses — [IndexableFields.Tiered.hashtags] - * and [IndexableFields.Tiered.locations] are filled here, so no branch can - * forget them. Values stay UNJOINED: separator choices belong to the backend. - */ private fun tiers( event: Event, primary: List, secondary: List, text: String?, websites: List = emptyList(), + ) = build(event, cleanAll(primary), cleanAll(secondary), text, cleanAll(websites)) + + /** + * The one funnel BOTH [tiers] overloads end in — [IndexableFields.Tiered.hashtags] + * and [IndexableFields.Tiered.locations] are filled here, so no branch can + * forget them. Values stay UNJOINED: separator choices belong to the backend. + */ + private fun build( + event: Event, + primary: List, + secondary: List, + text: String?, + websites: List, ) = IndexableFields.Tiered( - primary = cleanAll(primary), - secondary = cleanAll(secondary), + primary = primary, + secondary = secondary, text = clean(text), - hashtags = cleanAll(event.tags.hashtags()), + hashtags = hashtagValues(event), locations = locationValues(event), - websites = cleanAll(websites), + websites = websites, ) /** Trim and drop empties at the single funnel every derived string passes through. */ private fun clean(s: String?): String? = s?.trim()?.ifEmpty { null } - private fun cleanAll(parts: List): List = parts.mapNotNull { clean(it) } + private fun cleanOne(s: String?): List = clean(s)?.let { listOf(it) } ?: emptyList() + + /** + * Collects lazily: a role whose values are all absent — the common case on + * most kinds — costs no list at all. + */ + private fun cleanAll(parts: List): List { + var values: MutableList? = null + for (i in parts.indices) { + val value = clean(parts[i]) ?: continue + (values ?: ArrayList(parts.size).also { values = it }).add(value) + } + return values ?: emptyList() + } + + /** + * The hashtag role. [hashtags] allocates unconditionally, so the scan for + * a `t` tag comes first — most events carry none. The guard is exactly + * [HashtagTag.parse]'s own acceptance test, so it can never skip a tag the + * accessor would have returned. + */ + private fun hashtagValues(event: Event): List = if (!event.tags.fastAny(HashtagTag::isTagged)) emptyList() else cleanAll(event.tags.hashtags()) /** * Every `location` tag value, on ANY kind. Deliberately a raw scan, not a * typed accessor: Quartz's LocationTag classes are per-NIP (calendar, * picture, classifieds) and only those kinds expose locations(), while * this funnel must also catch location tags on kinds whose class doesn't - * model them. + * model them. Collects lazily, like [cleanAll]: an event with no location + * tag — nearly all of them — allocates nothing here. */ - private fun locationValues(event: Event): List = event.tags.mapNotNull { tag -> if (tag.getOrNull(0) != "location") null else clean(tag.getOrNull(1)) } + private fun locationValues(event: Event): List { + var values: MutableList? = null + event.tags.fastForEach { tag -> + if (tag.size > 1 && tag[0] == LOCATION_TAG) { + val value = clean(tag[1]) + if (value != null) { + (values ?: ArrayList(2).also { values = it }).add(value) + } + } + } + return values ?: emptyList() + } + + private const val LOCATION_TAG = "location" } diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractorTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractorTest.kt index 227b8b531c..3075095a39 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractorTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractorTest.kt @@ -298,24 +298,52 @@ class SearchFieldExtractorTest { @Test fun birdSightingsAreNamedByTheirSpeciesUnderBothNames() { - // Birdstar's `alt` is a boilerplate wrapper around the two names, so - // once commonName() has parsed it there is nothing left in it to - // index -- carrying it whole would repeat both names in a second role. + // The scientific name comes from the `n` tag, the vernacular one from + // commonName()'s parse of the `alt` -- both are names, so both are + // titles. The alt still reaches the summary tier whole. val tags = arrayOf( arrayOf("n", "Porphyrio martinica"), arrayOf("alt", "Bird detection: Purple Gallinule (Porphyrio martinica)"), ) val fields = SearchFieldExtractor.extract(BirdDetectionEvent("26".repeat(32), alice, 1L, tags, "", "")) - assertEquals(IndexableFields.Tiered(primary = listOf("Porphyrio martinica", "Purple Gallinule")), fields) + assertEquals( + IndexableFields.Tiered( + primary = listOf("Porphyrio martinica", "Purple Gallinule"), + secondary = listOf("Bird detection: Purple Gallinule (Porphyrio martinica)"), + ), + fields, + ) + } + + @Test + fun aBirdSightingKeepsAltTextBeyondTheParsedNames() { + // commonName() only matches a PREFIX, so a publisher can write + // anything after the parenthetical. Dropping the alt whenever that + // prefix parsed lost the tail ("at Lake Merritt, 7am") from every + // role, while indexableContent() still carried it -- the exact drift + // this extractor exists to prevent. + val tags = + arrayOf( + arrayOf("n", "Porphyrio martinica"), + arrayOf("alt", "Bird detection: Purple Gallinule (Porphyrio martinica) at Lake Merritt, 7am"), + ) + val fields = SearchFieldExtractor.extract(BirdDetectionEvent("3a".repeat(32), alice, 1L, tags, "", "")) + assertEquals( + IndexableFields.Tiered( + primary = listOf("Porphyrio martinica", "Purple Gallinule"), + secondary = listOf("Bird detection: Purple Gallinule (Porphyrio martinica) at Lake Merritt, 7am"), + ), + fields, + ) } @Test fun aBirdSightingWithAnUnrecognizedAltStillIndexesIt() { - // A publisher that words the alt differently keeps it: the summary is - // dropped only when commonName() proves it was the boilerplate. + // An alt that does not start with the known prefix parses to no + // common name, and is carried as the summary it is. val tags = arrayOf(arrayOf("n", "Ramphastos toco"), arrayOf("alt", "a toucan at the feeder")) - val fields = SearchFieldExtractor.extract(BirdDetectionEvent("3a".repeat(32), alice, 1L, tags, "", "")) + val fields = SearchFieldExtractor.extract(BirdDetectionEvent("3c".repeat(32), alice, 1L, tags, "", "")) assertEquals( IndexableFields.Tiered(primary = listOf("Ramphastos toco"), secondary = listOf("a toucan at the feeder")), fields, From fa3287f73719e64cda70983d35ebf311b4e51add Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 19:38:00 +0000 Subject: [PATCH 07/18] fix(quartz): match java.net.URLEncoder on native; drop the urlencoder dep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both native targets delegated UrlEncoder to net.thauvin.erik.urlencoder.UrlEncoderUtil, which implements RFC 3986 percent-encoding. The JVM/Android actual is java.net.URLEncoder/URLDecoder, which implements application/x-www-form-urlencoded. Different specifications, and the difference was observable: JVM/Android UrlEncoderUtil encode(" ") "+" "%20" encode("*") "*" "%2A" decode("a+b") "a b" "a+b" This is not cosmetic. encode() builds strings that leave the device — TorrentEvent puts it in magnet links, Nip54InlineMetadata in inline metadata, Nip47DeepLink in the callback/appname/value parameters of NWC deep links — so Android and iOS emitted different bytes for the same title. The decode row is worse: a link written by Android carries '+' for its spaces, and reading it on iOS or desktop-native gave back literal plus signs, silently, with no error. Replaced with one UrlEncoder.native.kt in nativeMain, shared by linuxX64 and every Apple target, matching URLEncoder/URLDecoder exactly — unreserved set is alphanumerics plus -_.* (note '*' survives and '~' does not, the opposite of RFC 3986), space to '+', uppercase %XX of UTF-8 bytes otherwise, and '+' back to space on the way in. Escape runs are encoded and decoded as runs so surrogate pairs and multi-byte sequences survive, and both directions short-circuit on a string with nothing to change, as the java.net pair does. UriParser.linux now delegates to UrlEncoder.decode rather than carrying its own copy of the decoder added in the previous commit. The new UrlEncoderTest lives in commonTest, so it pins every target against the JVM's answers — it is what found all three rows above, by passing on jvmTest and failing three of ten on linuxX64. net.thauvin.erik:urlencoder-lib had no other user and is removed from both source sets and the version catalog. One deliberate edge difference from the JVM, documented at the call site: an unpaired UTF-16 surrogate encodes as %EF%BF%BD rather than %3F. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HxQ1QuyzSkR38iFHbREjoS --- gradle/libs.versions.toml | 2 - quartz/build.gradle.kts | 2 - .../quartz/utils/UrlEncoder.apple.kt | 29 --- .../quartz/utils/UrlEncoderTest.kt | 134 ++++++++++++ .../quartz/utils/UriParser.linux.kt | 93 +-------- .../quartz/utils/UrlEncoder.linux.kt | 29 --- .../quartz/utils/UrlEncoder.native.kt | 190 ++++++++++++++++++ 7 files changed, 333 insertions(+), 146 deletions(-) delete mode 100644 quartz/src/appleMain/kotlin/com/vitorpamplona/quartz/utils/UrlEncoder.apple.kt create mode 100644 quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/utils/UrlEncoderTest.kt delete mode 100644 quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/UrlEncoder.linux.kt create mode 100644 quartz/src/nativeMain/kotlin/com/vitorpamplona/quartz/utils/UrlEncoder.native.kt diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index f19b09c9ca..baeabf40c1 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -55,7 +55,6 @@ media3 = "1.11.0" mockk = "1.14.11" kotlinx-coroutines-test = "1.11.0" negentropyKmp = "v1.2.0" -netUrlencoderLibVersion = "1.6.0" navigationCompose = "2.9.8" okhttp = "5.5.0" osmdroid = "6.1.20" @@ -209,7 +208,6 @@ mockk = { group = "io.mockk", name = "mockk", version.ref = "mockk" } mockk-android = { group = "io.mockk", name = "mockk-android", version.ref = "mockk" } kotlinx-coroutines-test = { group = "org.jetbrains.kotlinx", name = "kotlinx-coroutines-test", version.ref = "kotlinx-coroutines-test"} negentropy-kmp = { module = "com.vitorpamplona.negentropy:kmp-negentropy", version.ref = "negentropyKmp" } -net-thauvin-erik-urlencoder-lib = { module = "net.thauvin.erik.urlencoder:urlencoder-lib", version.ref = "netUrlencoderLibVersion" } okhttp = { group = "com.squareup.okhttp3", name = "okhttp", version.ref = "okhttp" } okhttpCoroutines = { group = "com.squareup.okhttp3", name = "okhttp-coroutines", version.ref = "okhttp" } osmdroid-android = { group = "org.osmdroid", name = "osmdroid-android", version.ref = "osmdroid" } diff --git a/quartz/build.gradle.kts b/quartz/build.gradle.kts index 4771e49055..9253f2c607 100644 --- a/quartz/build.gradle.kts +++ b/quartz/build.gradle.kts @@ -289,7 +289,6 @@ kotlin { dependsOn(nativeMain) dependencies { implementation(libs.charlietap.cachemap) - implementation(libs.net.thauvin.erik.urlencoder.lib) implementation(libs.dev.whyoleg.cryptography.provider.apple.optimal) implementation("io.github.andreypfau:kotlinx-crypto-hmac:0.0.4") implementation("io.github.andreypfau:kotlinx-crypto-sha2:0.0.4") @@ -347,7 +346,6 @@ kotlin { create("linuxMain") { dependsOn(nativeMain) dependencies { - implementation(libs.net.thauvin.erik.urlencoder.lib) implementation(libs.dev.whyoleg.cryptography.provider.apple.optimal) } } diff --git a/quartz/src/appleMain/kotlin/com/vitorpamplona/quartz/utils/UrlEncoder.apple.kt b/quartz/src/appleMain/kotlin/com/vitorpamplona/quartz/utils/UrlEncoder.apple.kt deleted file mode 100644 index 3b8e98f026..0000000000 --- a/quartz/src/appleMain/kotlin/com/vitorpamplona/quartz/utils/UrlEncoder.apple.kt +++ /dev/null @@ -1,29 +0,0 @@ -/* - * Copyright (c) 2025 Vitor Pamplona - * - * Permission is hereby granted, free of charge, to any person obtaining a copy of - * this software and associated documentation files (the "Software"), to deal in - * the Software without restriction, including without limitation the rights to use, - * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the - * Software, and to permit persons to whom the Software is furnished to do so, - * subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in all - * copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS - * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR - * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN - * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION - * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. - */ -package com.vitorpamplona.quartz.utils - -import net.thauvin.erik.urlencoder.UrlEncoderUtil - -actual object UrlEncoder { - actual fun encode(value: String): String = UrlEncoderUtil.encode(value) - - actual fun decode(value: String): String = UrlEncoderUtil.decode(value) -} diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/utils/UrlEncoderTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/utils/UrlEncoderTest.kt new file mode 100644 index 0000000000..33dc069b55 --- /dev/null +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/utils/UrlEncoderTest.kt @@ -0,0 +1,134 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.utils + +import kotlin.test.Test +import kotlin.test.assertEquals + +/** + * Cross-target contract for [UrlEncoder]. + * + * This is not a style preference — [UrlEncoder.encode] builds strings that leave the + * device. `TorrentEvent` puts it in magnet links, `Nip54InlineMetadata` in inline + * metadata, and `Nip47DeepLink` in the `callback`/`appname`/`value` parameters of NWC + * deep links. If Android encodes a title one way and iOS another, the two clients emit + * different bytes for the same event, and a wallet that round-trips a deep link built + * on one platform can fail on the other. + * + * The JVM/Android actual is `java.net.URLEncoder`/`URLDecoder` with UTF-8, so that is + * the reference every other target has to match. The expectations below are its + * `application/x-www-form-urlencoded` rules, which differ from plain RFC 3986 + * percent-encoding in exactly the three places a generic library gets "wrong": space, + * `*` and `~`. + */ +class UrlEncoderTest { + @Test + fun keepsTheUnreservedSet() { + // URLEncoder's dontNeedEncoding set is alphanumerics plus these four, and only + // these four. Note `*` survives and `~` does not — the opposite of RFC 3986. + assertEquals("abcXYZ019", UrlEncoder.encode("abcXYZ019")) + assertEquals("-_.*", UrlEncoder.encode("-_.*")) + } + + @Test + fun encodesSpaceAsPlus() { + // Form encoding, not %20. + assertEquals("hello+world", UrlEncoder.encode("hello world")) + assertEquals("a+b+c", UrlEncoder.encode("a b c")) + } + + @Test + fun encodesTildeAndTheOtherSubDelimiters() { + assertEquals("%7E", UrlEncoder.encode("~")) + assertEquals("%21", UrlEncoder.encode("!")) + assertEquals("%27", UrlEncoder.encode("'")) + assertEquals("%28%29", UrlEncoder.encode("()")) + assertEquals("%24%2C%3B", UrlEncoder.encode("$,;")) + } + + @Test + fun encodesUriPunctuationWithUppercaseHex() { + assertEquals("%3A%2F%2F", UrlEncoder.encode("://")) + assertEquals("%2B", UrlEncoder.encode("+")) + assertEquals("%3F%26%3D", UrlEncoder.encode("?&=")) + assertEquals("%40%23%25", UrlEncoder.encode("@#%")) + } + + @Test + fun encodesNonAsciiAsUtf8() { + assertEquals("%C3%A9", UrlEncoder.encode("é")) + assertEquals("caf%C3%A9", UrlEncoder.encode("café")) + assertEquals("%E2%82%AC", UrlEncoder.encode("€")) + // Outside the BMP: a surrogate pair has to encode as one 4-byte sequence. + assertEquals("%F0%9F%98%80", UrlEncoder.encode("😀")) + } + + @Test + fun decodesPlusAsSpace() { + assertEquals("hello world", UrlEncoder.decode("hello+world")) + assertEquals("hello world", UrlEncoder.decode("hello%20world")) + } + + @Test + fun decodesPercentEscapes() { + assertEquals("://", UrlEncoder.decode("%3A%2F%2F")) + assertEquals("~", UrlEncoder.decode("%7E")) + assertEquals("café", UrlEncoder.decode("caf%C3%A9")) + assertEquals("€", UrlEncoder.decode("%E2%82%AC")) + assertEquals("😀", UrlEncoder.decode("%F0%9F%98%80")) + // Lowercase hex decodes the same as uppercase. + assertEquals("é", UrlEncoder.decode("%c3%a9")) + } + + @Test + fun leavesUnescapedTextAlone() { + assertEquals("plain", UrlEncoder.decode("plain")) + assertEquals("-_.*~", UrlEncoder.decode("-_.*~")) + } + + @Test + fun roundTripsTheStringsThisIsActuallyUsedFor() { + // A torrent title (TorrentEvent) and a tracker URL. + val title = "Big Buck Bunny (2008) [1080p] ~ 60% done!" + assertEquals(title, UrlEncoder.decode(UrlEncoder.encode(title))) + + val tracker = "udp://tracker.example.org:1337/announce" + assertEquals("udp%3A%2F%2Ftracker.example.org%3A1337%2Fannounce", UrlEncoder.encode(tracker)) + assertEquals(tracker, UrlEncoder.decode(UrlEncoder.encode(tracker))) + + // An NWC pairing code (Nip47DeepLink.buildCallbackUri puts this in `value=`). + val pairing = + "nostr+walletconnect://b889ff5b1513b641e2a139f661a661364979c5beee91842f8f0ef42ab558e9d4" + + "?relay=wss%3A%2F%2Frelay.damus.io&secret=71a8c14c1407c113601079c4302dab36460f0ccd0ad506f1f2dc73b5100571c5" + assertEquals(pairing, UrlEncoder.decode(UrlEncoder.encode(pairing))) + + // A callback deep link (Nip47DeepLink.parseConnectUri reads this back). + val callback = "amethystnwc://callback" + assertEquals("amethystnwc%3A%2F%2Fcallback", UrlEncoder.encode(callback)) + assertEquals(callback, UrlEncoder.decode(UrlEncoder.encode(callback))) + } + + @Test + fun roundTripsEveryAsciiCharacter() { + val ascii = (0..127).map { it.toChar() }.joinToString("") + assertEquals(ascii, UrlEncoder.decode(UrlEncoder.encode(ascii))) + } +} diff --git a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/UriParser.linux.kt b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/UriParser.linux.kt index 448f4b8c8f..59c8a40b7d 100644 --- a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/UriParser.linux.kt +++ b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/UriParser.linux.kt @@ -122,9 +122,13 @@ actual class UriParser actual constructor( actual fun path(): String? = parsedPath /** - * Parsed once and reused, mirroring the JVM actual's lazy map. The previous version - * re-split the entire query string on every [getQueryParameter] call, so a URI read - * for four parameters was parsed four times. + * Parsed once and reused, mirroring the JVM actual's lazy map — the previous version + * re-split the entire query string on every [getQueryParameter] call. + * + * Decoded with [UrlEncoder.decode], which matches `URLDecoder.decode(.., "UTF-8")` — + * what the JVM actual calls. Skipping this is why NIP-47 failed on this target: + * `relay=wss%3A%2F%2Frelay.damus.io` reached `RelayUrlNormalizer` still encoded and + * came back "Invalid relay Url". */ private val queryParameters: Map> by lazy { parsedQuery?.ifBlank { null }?.let { query -> @@ -138,7 +142,7 @@ actual class UriParser actual constructor( } if (parts.size == 2) { - currentValue.add(percentDecode(parts[1])) + currentValue.add(UrlEncoder.decode(parts[1])) } else { currentValue.add("") } @@ -153,7 +157,7 @@ actual class UriParser actual constructor( keyValuePair.split('&').associate { paramValue -> val parts = paramValue.split("=", limit = 2) if (parts.size == 2) { - parts[0] to percentDecode(parts[1]) + parts[0] to UrlEncoder.decode(parts[1]) } else { parts[0] to "" // Handle parameters without a value } @@ -168,82 +172,3 @@ actual class UriParser actual constructor( actual fun fragments(): Map = parsedFragments } - -/** - * `java.net.URLDecoder.decode(value, "UTF-8")` — which is literally what the JVM actual - * calls — for a target with no `java.net`. - * - * This is the whole reason NIP-47 failed on linuxX64: the parser returned query values - * exactly as they appeared in the URI, so `relay=wss%3A%2F%2Frelay.damus.io` reached - * `RelayUrlNormalizer` still percent-encoded and came back "Invalid relay Url". Decoding - * belongs here rather than in each caller, because the JVM and Apple actuals both hand - * back decoded values and common code is written against that. - * - * Matches `URLDecoder` in the details that are observable: `+` becomes a space, a run of - * consecutive `%XX` is decoded as one UTF-8 sequence (so multi-byte characters survive), - * every other character passes through, and a malformed escape throws - * [IllegalArgumentException] rather than being silently kept — the same failure the JVM - * gives for the same input. - */ -private fun percentDecode(value: String): String { - // The short-circuit URLDecoder also makes: with nothing to change, return the - // original instance rather than rebuilding it. Most query values hit this. - if (value.indexOf('%') < 0 && value.indexOf('+') < 0) return value - - val result = StringBuilder(value.length) - var index = 0 - // Sized on first use for the longest run that could still follow, then reused — - // one allocation for the whole string, as in URLDecoder. - var escaped: ByteArray? = null - - while (index < value.length) { - when (val char = value[index]) { - '+' -> { - result.append(' ') - index++ - } - - '%' -> { - val buffer = escaped ?: ByteArray((value.length - index) / 3).also { escaped = it } - var count = 0 - while (index + 2 < value.length && value[index] == '%') { - buffer[count++] = decodeEscape(value, index) - index += 3 - } - if (index < value.length && value[index] == '%') { - throw IllegalArgumentException("URLDecoder: Incomplete trailing escape (%) pattern") - } - result.append(buffer.decodeToString(0, count)) - } - - else -> { - result.append(char) - index++ - } - } - } - - return result.toString() -} - -private fun decodeEscape( - value: String, - index: Int, -): Byte { - val high = hexDigit(value[index + 1]) - val low = hexDigit(value[index + 2]) - if (high < 0 || low < 0) { - throw IllegalArgumentException( - "URLDecoder: Illegal hex characters in escape (%) pattern - ${value.substring(index, index + 3)}", - ) - } - return ((high shl 4) or low).toByte() -} - -private fun hexDigit(char: Char): Int = - when (char) { - in '0'..'9' -> char - '0' - in 'a'..'f' -> char - 'a' + 10 - in 'A'..'F' -> char - 'A' + 10 - else -> -1 - } diff --git a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/UrlEncoder.linux.kt b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/UrlEncoder.linux.kt deleted file mode 100644 index 3b8e98f026..0000000000 --- a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/UrlEncoder.linux.kt +++ /dev/null @@ -1,29 +0,0 @@ -/* - * Copyright (c) 2025 Vitor Pamplona - * - * Permission is hereby granted, free of charge, to any person obtaining a copy of - * this software and associated documentation files (the "Software"), to deal in - * the Software without restriction, including without limitation the rights to use, - * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the - * Software, and to permit persons to whom the Software is furnished to do so, - * subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in all - * copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS - * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR - * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN - * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION - * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. - */ -package com.vitorpamplona.quartz.utils - -import net.thauvin.erik.urlencoder.UrlEncoderUtil - -actual object UrlEncoder { - actual fun encode(value: String): String = UrlEncoderUtil.encode(value) - - actual fun decode(value: String): String = UrlEncoderUtil.decode(value) -} diff --git a/quartz/src/nativeMain/kotlin/com/vitorpamplona/quartz/utils/UrlEncoder.native.kt b/quartz/src/nativeMain/kotlin/com/vitorpamplona/quartz/utils/UrlEncoder.native.kt new file mode 100644 index 0000000000..2f1b1873e9 --- /dev/null +++ b/quartz/src/nativeMain/kotlin/com/vitorpamplona/quartz/utils/UrlEncoder.native.kt @@ -0,0 +1,190 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.utils + +/** + * Native actual for [UrlEncoder], shared by linuxX64 and every Apple target. + * + * ## Why this is not a library call any more + * + * Both native targets used to delegate to `net.thauvin.erik.urlencoder.UrlEncoderUtil`, + * which implements RFC 3986 percent-encoding. The JVM/Android actual is + * `java.net.URLEncoder`/`URLDecoder`, which implements + * `application/x-www-form-urlencoded`. Those are different specifications, and the + * difference was observable in three places: + * + * ``` + * JVM/Android UrlEncoderUtil + * encode(" ") "+" "%20" + * encode("*") "*" "%2A" + * decode("a+b") "a b" "a+b" + * ``` + * + * That is not cosmetic. [encode] builds strings that leave the device — `TorrentEvent` + * puts it in magnet links, `Nip54InlineMetadata` in inline metadata, `Nip47DeepLink` in + * the `callback`, `appname` and `value` parameters of NWC deep links — so Android and + * iOS were emitting different bytes for the same title. The decode row is worse than + * cosmetic: a magnet link or deep link written by Android carries `+` for its spaces, + * and reading it on iOS produced a string with literal plus signs instead of spaces, no + * error anywhere. + * + * So this matches `URLEncoder`/`URLDecoder` exactly instead: the unreserved set is + * alphanumerics plus `-`, `_`, `.` and `*` (note `*` survives and `~` does not — the + * opposite of RFC 3986), space encodes to `+`, everything else to uppercase `%XX` of + * its UTF-8 bytes, and decoding maps `+` back to a space. `UrlEncoderTest` in + * `commonTest` pins all of it against the JVM on every target. + * + * Both directions short-circuit the way the `java.net` pair does: a string with nothing + * to change is returned as-is rather than rebuilt. + * + * One deliberate edge difference: an *unpaired* UTF-16 surrogate encodes as `%EF%BF%BD` + * (Kotlin's replacement character) where the JVM gives `%3F`. Nostr content is + * well-formed UTF-16, and chasing it would cost a scan on every call. + */ +actual object UrlEncoder { + private const val HEX = "0123456789ABCDEF" + + actual fun encode(value: String): String { + var index = 0 + while (index < value.length && isUnreserved(value[index])) index++ + if (index == value.length) return value + + val result = StringBuilder(value.length + ESCAPE_HEADROOM) + result.append(value, 0, index) + + while (index < value.length) { + val char = value[index] + when { + isUnreserved(char) -> { + result.append(char) + index++ + } + + char == ' ' -> { + result.append('+') + index++ + } + + else -> { + // Escaped as a run rather than character by character, so a surrogate + // pair becomes one 4-byte sequence instead of two malformed 3-byte ones. + val start = index + do { + index++ + } while (index < value.length && !isUnreserved(value[index]) && value[index] != ' ') + appendEscaped(result, value, start, index) + } + } + } + + return result.toString() + } + + actual fun decode(value: String): String { + if (value.indexOf('%') < 0 && value.indexOf('+') < 0) return value + + val result = StringBuilder(value.length) + var index = 0 + // Sized on first use for the longest run that could still follow, then reused — + // one allocation for the whole string, as in URLDecoder. + var escaped: ByteArray? = null + + while (index < value.length) { + when (val char = value[index]) { + '+' -> { + result.append(' ') + index++ + } + + '%' -> { + val buffer = escaped ?: ByteArray((value.length - index) / 3).also { escaped = it } + var count = 0 + while (index + 2 < value.length && value[index] == '%') { + buffer[count++] = decodeEscape(value, index) + index += 3 + } + if (index < value.length && value[index] == '%') { + throw IllegalArgumentException("URLDecoder: Incomplete trailing escape (%) pattern") + } + // Decoded as a run so a multi-byte UTF-8 sequence survives. + result.append(buffer.decodeToString(0, count)) + } + + else -> { + result.append(char) + index++ + } + } + } + + return result.toString() + } + + /** `URLEncoder`'s `dontNeedEncoding` set: alphanumerics plus these four, and only these. */ + private fun isUnreserved(char: Char): Boolean = + char in 'a'..'z' || + char in 'A'..'Z' || + char in '0'..'9' || + char == '-' || + char == '_' || + char == '.' || + char == '*' + + private fun appendEscaped( + result: StringBuilder, + value: String, + start: Int, + end: Int, + ) { + val bytes = value.substring(start, end).encodeToByteArray() + for (byte in bytes) { + val code = byte.toInt() + result.append('%') + result.append(HEX[(code shr 4) and 0xF]) + result.append(HEX[code and 0xF]) + } + } + + private fun decodeEscape( + value: String, + index: Int, + ): Byte { + val high = hexDigit(value[index + 1]) + val low = hexDigit(value[index + 2]) + if (high < 0 || low < 0) { + throw IllegalArgumentException( + "URLDecoder: Illegal hex characters in escape (%) pattern - ${value.substring(index, index + 3)}", + ) + } + return ((high shl 4) or low).toByte() + } + + private fun hexDigit(char: Char): Int = + when (char) { + in '0'..'9' -> char - '0' + in 'a'..'f' -> char - 'a' + 10 + in 'A'..'F' -> char - 'A' + 10 + else -> -1 + } + + /** Enough for a handful of escapes before the builder has to grow. */ + private const val ESCAPE_HEADROOM = 16 +} From d18b7f770fe2805afb04094f39de5f54b7ecf851 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Tue, 1 Sep 2026 15:40:48 -0400 Subject: [PATCH 08/18] perf(feed): defer animation transitions until there is something to animate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `updateTransition` and `AnimatedContent` allocate a Transition, its animation list and its seeking state on *first* composition — but first composition has nothing to animate, because target and initial state are the same value. In a feed that is waste: every card scrolled in built six of them, and during a scroll essentially none ever ran, since reaction counts and icons do not change in the second a card is on screen. `DeferredCrossfade` and `DeferredAnimatedContent` render the plain content until the target actually moves, then build the transition seeded at the *original* value via `MutableTransitionState` and immediately re-target it — so the first real change still animates exactly as before, and later changes animate through the now-live transition normally. The existing `isPerformanceMode()` branch, which genuinely drops the animation, is untouched and still takes precedence. Measured on an SM-T220 against a frozen corpus served by a local relay (a real capture: 105 notes, 68 profiles, 501 reactions, 75 boosts, 22 zaps), interleaved with the unmodified build, two runs per arm: frame duration P90 27.53 -> 26.90 -2.3% (baseline spread 0.1%) frame overrun P90 21.65 -> 17.44 -19.4% (baseline spread 6.3%) frame duration P50 -1.1% (inside a 1.7% spread) Modest at the frame level by nature: on this device the main thread sits blocked in `postAndWait` on the RenderThread for roughly two-thirds of every frame, so composition savings largely do not surface. Removing 24 flow subscriptions per card, every clickable, or every counter each moved `postAndWait` by only ~2%. `DeferredAnimationTest` drives the clock manually and asserts the outgoing and incoming content coexist mid-transition, which only a running animation does; a regression turning the deferral into a snap fails it. --- .../amethyst/ui/note/DeferredAnimationTest.kt | 91 +++++++++++++++++++ .../amethyst/ui/actions/CrossfadeIfEnabled.kt | 50 +++++++++- .../amethyst/ui/note/ReactionsRow.kt | 55 +++++++++-- 3 files changed, 185 insertions(+), 11 deletions(-) create mode 100644 amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/note/DeferredAnimationTest.kt diff --git a/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/note/DeferredAnimationTest.kt b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/note/DeferredAnimationTest.kt new file mode 100644 index 0000000000..12b6892a0f --- /dev/null +++ b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/note/DeferredAnimationTest.kt @@ -0,0 +1,91 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.ui.note + +import androidx.compose.animation.core.tween +import androidx.compose.material3.Text +import androidx.compose.runtime.mutableStateOf +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.platform.testTag +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.junit4.createComposeRule +import androidx.compose.ui.test.onNodeWithTag +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.vitorpamplona.amethyst.ui.actions.DeferredCrossfade +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith + +/** + * The feed's animated elements defer building their `Transition` until a value actually changes, + * because first composition has nothing to animate and building one per card per scroll is pure + * waste (measured: roughly half the composition cost of every reaction-row button). + * + * The whole point of deferring rather than removing is that the animation must still play. These + * tests pin that: they drive the clock manually and assert that the **first** change — the one that + * happens right after the transition is lazily created — still shows outgoing and incoming content + * simultaneously, which only a running animation does. A regression that turned the deferral into a + * plain snap would show exactly one of them and fail here. + */ +@RunWith(AndroidJUnit4::class) +class DeferredAnimationTest { + @get:Rule + val rule = createComposeRule() + + @Test + fun deferredCrossfadeStillAnimatesTheFirstChange() { + val state = mutableStateOf("A") + rule.mainClock.autoAdvance = false + + rule.setContent { + DeferredCrossfade( + targetState = state.value, + modifier = Modifier, + contentAlignment = Alignment.TopStart, + animationSpec = tween(DURATION_MS), + label = "test", + ) { value -> + Text(value, modifier = Modifier.testTag("text_$value")) + } + } + + // Before any change the transition has not been built, and only the current value renders. + rule.onNodeWithTag("text_A").assertIsDisplayed() + rule.onNodeWithTag("text_B").assertDoesNotExist() + + state.value = "B" + rule.mainClock.advanceTimeByFrame() + rule.mainClock.advanceTimeBy(DURATION_MS / 3L) + + // Mid-crossfade both are in the tree. This is the assertion that a snap would fail. + rule.onNodeWithTag("text_A").assertExists() + rule.onNodeWithTag("text_B").assertExists() + + rule.mainClock.advanceTimeBy(DURATION_MS * 3L) + rule.onNodeWithTag("text_B").assertIsDisplayed() + rule.onNodeWithTag("text_A").assertDoesNotExist() + } + + companion object { + const val DURATION_MS = 300 + } +} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/actions/CrossfadeIfEnabled.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/actions/CrossfadeIfEnabled.kt index fc6be9550a..4c5014141f 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/actions/CrossfadeIfEnabled.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/actions/CrossfadeIfEnabled.kt @@ -23,8 +23,10 @@ package com.vitorpamplona.amethyst.ui.actions import androidx.collection.mutableScatterMapOf import androidx.compose.animation.ExperimentalAnimationApi import androidx.compose.animation.core.FiniteAnimationSpec +import androidx.compose.animation.core.MutableTransitionState import androidx.compose.animation.core.Transition import androidx.compose.animation.core.animateFloat +import androidx.compose.animation.core.rememberTransition import androidx.compose.animation.core.tween import androidx.compose.animation.core.updateTransition import androidx.compose.foundation.layout.Box @@ -54,7 +56,53 @@ fun CrossfadeIfEnabled( content(targetState) } } else { - MyCrossfade(targetState, modifier, contentAlignment, animationSpec, label, content) + DeferredCrossfade(targetState, modifier, contentAlignment, animationSpec, label, content) + } +} + +/** Latches the first time a crossfade's target moves off the value it was composed with. */ +private class ChangeLatch { + var changed = false +} + +/** + * A [MyCrossfade] that does not build its [androidx.compose.animation.core.Transition] until there + * is something to animate. + * + * `updateTransition` allocates a transition, its animation list and its seeking state on *first + * composition*, even though first composition has nothing to cross-fade — target and initial state + * are the same value. In a feed that is waste: every card scrolled in builds a transition per + * animated element, and during a scroll essentially none of them run, because the underlying counts + * and icons do not change in the second a card is on screen. + * + * So the plain content renders until the target actually moves. At that point the transition is + * built seeded at the *original* value via [MutableTransitionState] and immediately re-targeted at + * the new one, so the first real change still animates exactly as before; every later change + * animates through the now-live transition normally. + */ +@OptIn(ExperimentalAnimationApi::class) +@Composable +internal fun DeferredCrossfade( + targetState: T, + modifier: Modifier, + contentAlignment: Alignment, + animationSpec: FiniteAnimationSpec, + label: String, + content: @Composable (T) -> Unit, +) { + val initial = remember { targetState } + val latch = remember { ChangeLatch() } + if (targetState != initial) latch.changed = true + + if (!latch.changed) { + Box(modifier, contentAlignment) { + content(targetState) + } + } else { + val transitionState = remember { MutableTransitionState(initial) } + transitionState.targetState = targetState + val transition = rememberTransition(transitionState, label) + transition.MyCrossfade(modifier, contentAlignment, animationSpec, content = content) } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ReactionsRow.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ReactionsRow.kt index 8c31cfff8d..6ba63fe27f 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ReactionsRow.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ReactionsRow.kt @@ -31,6 +31,7 @@ import androidx.compose.animation.ExperimentalAnimationApi import androidx.compose.animation.animateColorAsState import androidx.compose.animation.core.MutableTransitionState import androidx.compose.animation.core.animateFloatAsState +import androidx.compose.animation.core.rememberTransition import androidx.compose.animation.core.tween import androidx.compose.animation.expandHorizontally import androidx.compose.animation.fadeIn @@ -857,11 +858,7 @@ private fun SlidingAnimationCount( if (accountViewModel.settings.isPerformanceMode()) { TextCount(baseCount, textColor) } else { - AnimatedContent( - targetState = baseCount, - transitionSpec = AnimatedContentTransitionScope::transitionSpec, - label = "SlidingAnimationCount", - ) { count -> + DeferredAnimatedContent(baseCount, "SlidingAnimationCount") { count -> TextCount(count, textColor) } } @@ -884,6 +881,48 @@ val slideAnimation: ContentTransform = ), ) +/** Latches the first time an animated counter's value moves off the one it was composed with. */ +private class CountChangeLatch { + var changed = false +} + +/** + * An [AnimatedContent] that does not build its transition until the value actually changes. + * + * Same reasoning as `DeferredCrossfade`: `AnimatedContent` builds a transition plus its content map + * and size animation on first composition, but first composition has nothing to animate. A reaction + * counter only slides when the count moves, which practically never happens in the second a card + * spends on screen during a scroll — so the apparatus was built and thrown away, once per counter + * per card. + * + * Rendering the bare content until the first change, then seeding a [MutableTransitionState] at the + * original value, keeps that first change animated exactly as before. + */ +@OptIn(ExperimentalAnimationApi::class) +@Composable +private fun DeferredAnimatedContent( + targetState: T, + label: String, + content: @Composable (T) -> Unit, +) { + val initial = remember { targetState } + val latch = remember { CountChangeLatch() } + if (targetState != initial) latch.changed = true + + if (!latch.changed) { + content(targetState) + } else { + val transitionState = remember { MutableTransitionState(initial) } + transitionState.targetState = targetState + val transition = rememberTransition(transitionState, label) + transition.AnimatedContent( + transitionSpec = { transitionSpec() }, + ) { value -> + content(value) + } + } +} + @Composable fun TextCount( count: Int, @@ -911,11 +950,7 @@ fun SlidingAnimationAmount( maxLines = 1, ) } else { - AnimatedContent( - targetState = amount, - transitionSpec = AnimatedContentTransitionScope::transitionSpec, - label = "SlidingAnimationAmount", - ) { count -> + DeferredAnimatedContent(amount, "SlidingAnimationAmount") { count -> Text( text = count, fontSize = Font14SP, From ebcdd9d3d5a872f486a2ebdc88a2a6dea7b33162 Mon Sep 17 00:00:00 2001 From: davotoula Date: Tue, 1 Sep 2026 08:45:16 +0200 Subject: [PATCH 09/18] feat(layout): pure tier and panel decisions keyed on window shape Introduces decideNavigationStyle/decideNotificationPanel with the shape rule from #4024, plus unit tests for the whole behaviour table. Not wired up yet - rememberScreenLayoutSpec is unchanged, so behaviour is identical. --- .../amethyst/ui/layouts/ScreenLayout.kt | 59 +++++++++ .../ui/layouts/ScreenLayoutSpecTest.kt | 113 ++++++++++++++++++ 2 files changed, 172 insertions(+) create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayoutSpecTest.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayout.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayout.kt index 57b66bc8b5..7de879fb58 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayout.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayout.kt @@ -26,6 +26,7 @@ import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.widthIn import androidx.compose.material3.MaterialTheme import androidx.compose.material3.windowsizeclass.ExperimentalMaterial3WindowSizeClassApi +import androidx.compose.material3.windowsizeclass.WindowSizeClass import androidx.compose.material3.windowsizeclass.WindowWidthSizeClass import androidx.compose.material3.windowsizeclass.calculateWindowSizeClass import androidx.compose.runtime.Composable @@ -35,6 +36,7 @@ import androidx.compose.runtime.remember import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.platform.LocalConfiguration +import androidx.compose.ui.unit.DpSize import androidx.compose.ui.unit.dp import com.vitorpamplona.amethyst.ui.components.getActivity @@ -95,6 +97,63 @@ val NotificationPanelWidth = 360.dp */ val FeedContentMaxWidth = 600.dp +/** + * Minimum window height for the docked drawer. Higher than Material's 480dp Compact/Medium + * height boundary on purpose: the permanent drawer's own header — banner, avatar, status + * editor, follower counts — fills most of a ~540dp column before the first navigation row, so + * below this the rail shows more of the menu than the dock does. + */ +private const val DOCK_MIN_WINDOW_HEIGHT_DP = 600 + +/** + * The navigation tier for a window of this shape. + * + * The dock is not a width decision. A tablet is past the Expanded breakpoint in both + * orientations, so keying on width alone pins 300dp of menu open in portrait with no closed + * state to fall back on (issue #4024). It docks only when the window is wide, landscape, and + * tall enough for the drawer's own content to be usable; everything else that is not Compact + * falls through to the rail, which pairs with the existing swipe-in modal drawer. + * + * Takes plain dp rather than a [WindowWidthSizeClass] so a caller cannot pass a size class + * that contradicts the size, and so the whole table is unit-testable without a composition. + */ +@OptIn(ExperimentalMaterial3WindowSizeClassApi::class) +internal fun decideNavigationStyle( + windowWidthDp: Int, + windowHeightDp: Int, +): NavigationStyle { + val widthSizeClass = + WindowSizeClass + .calculateFromSize(DpSize(windowWidthDp.dp, windowHeightDp.dp)) + .widthSizeClass + + return when { + widthSizeClass == WindowWidthSizeClass.Expanded && + windowWidthDp >= windowHeightDp && + windowHeightDp >= DOCK_MIN_WINDOW_HEIGHT_DP -> NavigationStyle.PERMANENT_DRAWER + widthSizeClass != WindowWidthSizeClass.Compact -> NavigationStyle.NAV_RAIL + else -> NavigationStyle.BOTTOM_BAR + } +} + +/** + * Whether the window is wide enough to dock the notification feed beside the content. + * + * Deliberately not keyed on [NavigationStyle.PERMANENT_DRAWER]: a wide portrait window now + * gets the rail, and gating on the dock would strip a panel it has today. Named + * `decideNotificationPanel` rather than `showsNotificationPanel` so it does not shadow + * [ScreenLayoutSpec.showsNotificationPanel] at the construction site. + * + * The [NavigationStyle.BOTTOM_BAR] term is not load-bearing — Compact is under 600dp, so a + * bottom-bar window cannot reach 1200dp — but it makes that an invariant the code enforces. + */ +internal fun decideNotificationPanel( + style: NavigationStyle, + windowWidthDp: Int, +): Boolean = + style != NavigationStyle.BOTTOM_BAR && + windowWidthDp >= NOTIFICATION_PANEL_MIN_WINDOW_DP + /** * Centers a destination's content at [FeedContentMaxWidth]. The outer box paints the theme * background so the gutters match the screens' own surfaces; on Compact windows the cap is diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayoutSpecTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayoutSpecTest.kt new file mode 100644 index 0000000000..5c846a9250 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayoutSpecTest.kt @@ -0,0 +1,113 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.ui.layouts + +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * The tier rule from amethyst/plans/2026-08-31-portrait-sidebar-tier-rule.md: + * + * dock <=> widthClass == Expanded && width >= height && height >= 600dp + * + * Sizes are window dp, width first. Every row of the spec's Behaviour table appears here. + */ +class ScreenLayoutSpecTest { + private fun assertStyle( + expected: NavigationStyle, + widthDp: Int, + heightDp: Int, + ) = assertEquals("${widthDp}x${heightDp}dp", expected, decideNavigationStyle(widthDp, heightDp)) + + // ---- Behaviour table ---- + + @Test + fun phonePortraitKeepsTheBottomBar() = assertStyle(NavigationStyle.BOTTOM_BAR, 411, 923) + + @Test + fun phoneLandscapeNoLongerDocks() = assertStyle(NavigationStyle.NAV_RAIL, 923, 411) + + @Test + fun reporterTabletPortraitNoLongerDocks() = assertStyle(NavigationStyle.NAV_RAIL, 889, 1422) + + @Test + fun wideTabletPortraitNoLongerDocks() = assertStyle(NavigationStyle.NAV_RAIL, 1201, 1920) + + @Test + fun mediumTabletPortraitStillRails() = assertStyle(NavigationStyle.NAV_RAIL, 800, 1280) + + @Test + fun tabletLandscapeStillDocks() = assertStyle(NavigationStyle.PERMANENT_DRAWER, 1422, 889) + + @Test + fun mediumTabletLandscapeStillDocks() = assertStyle(NavigationStyle.PERMANENT_DRAWER, 1280, 800) + + @Test + fun wideButShortLandscapeNoLongerDocks() = assertStyle(NavigationStyle.NAV_RAIL, 1200, 540) + + // ---- Boundaries ---- + + @Test + fun squareWindowAtTheWidthBreakpointDocks() = assertStyle(NavigationStyle.PERMANENT_DRAWER, 840, 840) + + @Test + fun portraitByOneDpDoesNotDock() = assertStyle(NavigationStyle.NAV_RAIL, 840, 841) + + @Test + fun exactlyAtTheHeightFloorDocks() = assertStyle(NavigationStyle.PERMANENT_DRAWER, 840, 600) + + @Test + fun oneDpBelowTheHeightFloorDoesNotDock() = assertStyle(NavigationStyle.NAV_RAIL, 840, 599) + + @Test + fun oneDpBelowTheWidthBreakpointRails() = assertStyle(NavigationStyle.NAV_RAIL, 839, 600) + + @Test + fun compactWidthKeepsTheBottomBar() = assertStyle(NavigationStyle.BOTTOM_BAR, 599, 900) + + // ---- Notification panel ---- + + @Test + fun panelHiddenJustBelowTheThreshold() = assertEquals(false, decideNotificationPanel(NavigationStyle.PERMANENT_DRAWER, 1199)) + + @Test + fun panelShownExactlyAtTheThreshold() = assertEquals(true, decideNotificationPanel(NavigationStyle.PERMANENT_DRAWER, 1200)) + + @Test + fun panelShownAboveTheThreshold() = assertEquals(true, decideNotificationPanel(NavigationStyle.PERMANENT_DRAWER, 1201)) + + @Test + fun panelSurvivesTheCollapseToTheRail() = assertEquals(true, decideNotificationPanel(NavigationStyle.NAV_RAIL, 1200)) + + @Test + fun panelNeverAppearsOnTheBottomBar() = assertEquals(false, decideNotificationPanel(NavigationStyle.BOTTOM_BAR, 1200)) + + /** + * A landscape-short window: wide enough for the panel, too short for the dock. Rail plus + * panel is a combination that has never shipped, so assert both halves together. + */ + @Test + fun shortLandscapeYieldsRailPlusPanel() { + val style = decideNavigationStyle(1200, 599) + assertEquals(NavigationStyle.NAV_RAIL, style) + assertEquals(true, decideNotificationPanel(style, 1200)) + } +} From d9c10f4abbc201746444fd55c9da0084011fbcaa Mon Sep 17 00:00:00 2001 From: davotoula Date: Tue, 1 Sep 2026 10:11:41 +0200 Subject: [PATCH 10/18] Code reviews: - refactor(layout): one multi-pane shell, and name the panel predicate honestly - fix(layout): don't dock the sidebar on portrait tablets (#4024) --- .../amethyst/ui/components/WindowUtils.kt | 3 - .../amethyst/ui/layouts/ScreenLayout.kt | 61 +++++------- .../AccountSwitcherAndLeftDrawerLayout.kt | 99 ++++++++++++++----- ...nLayoutSpecTest.kt => ScreenLayoutTest.kt} | 22 ++--- 4 files changed, 101 insertions(+), 84 deletions(-) rename amethyst/src/test/java/com/vitorpamplona/amethyst/ui/layouts/{ScreenLayoutSpecTest.kt => ScreenLayoutTest.kt} (80%) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/WindowUtils.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/WindowUtils.kt index e9e27deb53..4bda918632 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/WindowUtils.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/WindowUtils.kt @@ -45,9 +45,6 @@ private tailrec fun Context.getActivityWindow(): Window? = else -> null } -@Composable -fun getActivity(): Activity = LocalContext.current.getActivity() - tailrec fun Context.getActivity(): ComponentActivity = when (this) { is ComponentActivity -> this diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayout.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayout.kt index 7de879fb58..51ceb43c1e 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayout.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayout.kt @@ -28,7 +28,6 @@ import androidx.compose.material3.MaterialTheme import androidx.compose.material3.windowsizeclass.ExperimentalMaterial3WindowSizeClassApi import androidx.compose.material3.windowsizeclass.WindowSizeClass import androidx.compose.material3.windowsizeclass.WindowWidthSizeClass -import androidx.compose.material3.windowsizeclass.calculateWindowSizeClass import androidx.compose.runtime.Composable import androidx.compose.runtime.Immutable import androidx.compose.runtime.compositionLocalOf @@ -38,7 +37,6 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.platform.LocalConfiguration import androidx.compose.ui.unit.DpSize import androidx.compose.ui.unit.dp -import com.vitorpamplona.amethyst.ui.components.getActivity /** How the app shell presents its top-level navigation for the current window size. */ enum class NavigationStyle { @@ -46,12 +44,13 @@ enum class NavigationStyle { BOTTOM_BAR, /** - * Medium windows (portrait tablets, unfolded foldables): a left navigation rail - * replaces the bottom bar; the drawer stays modal behind the rail's avatar button. + * Every non-Compact window that does not dock — portrait tablets and unfolded foldables at + * any width, plus short landscape windows: a left navigation rail replaces the bottom bar + * and the drawer stays modal behind the rail's avatar button. */ NAV_RAIL, - /** Expanded windows (landscape tablets, desktop windows): the drawer docks permanently on the left. */ + /** Wide, landscape, tall windows (landscape tablets, desktop): the drawer docks permanently on the left. */ PERMANENT_DRAWER, } @@ -62,7 +61,7 @@ enum class NavigationStyle { @Immutable data class ScreenLayoutSpec( val navigationStyle: NavigationStyle, - val showsNotificationPanel: Boolean, + val hasRoomForNotificationPanel: Boolean, ) { /** * True on the rail and permanent-drawer tiers. Large screens hide the bottom bar and pin @@ -71,16 +70,18 @@ data class ScreenLayoutSpec( val isLargeScreen: Boolean get() = navigationStyle != NavigationStyle.BOTTOM_BAR companion object { - val Phone = ScreenLayoutSpec(NavigationStyle.BOTTOM_BAR, showsNotificationPanel = false) + val Phone = ScreenLayoutSpec(NavigationStyle.BOTTOM_BAR, hasRoomForNotificationPanel = false) } } val LocalScreenLayout = compositionLocalOf { ScreenLayoutSpec.Phone } /** - * Minimum window width for the docked notification panel: the permanent drawer - * ([PermanentDrawerWidth]) + a readable center pane + the panel ([NotificationPanelWidth]) - * only coexist comfortably from a landscape-tablet-sized window up. + * Minimum window width for the docked notification panel: a leading navigation pane, a + * readable center pane and the panel ([NotificationPanelWidth]) only coexist comfortably from + * a landscape-tablet-sized window up. Sized against the widest leading pane, the permanent + * drawer ([PermanentDrawerWidth]); the rail is narrower, so a railed window that clears this + * gets a roomier center pane rather than a tighter one. */ private const val NOTIFICATION_PANEL_MIN_WINDOW_DP = 1200 @@ -114,8 +115,8 @@ private const val DOCK_MIN_WINDOW_HEIGHT_DP = 600 * tall enough for the drawer's own content to be usable; everything else that is not Compact * falls through to the rail, which pairs with the existing swipe-in modal drawer. * - * Takes plain dp rather than a [WindowWidthSizeClass] so a caller cannot pass a size class - * that contradicts the size, and so the whole table is unit-testable without a composition. + * A square window counts as landscape and docks; `Configuration.ORIENTATION_LANDSCAPE` + * breaks that tie the other way, so the two disagree at exactly width == height. */ @OptIn(ExperimentalMaterial3WindowSizeClassApi::class) internal fun decideNavigationStyle( @@ -139,20 +140,10 @@ internal fun decideNavigationStyle( /** * Whether the window is wide enough to dock the notification feed beside the content. * - * Deliberately not keyed on [NavigationStyle.PERMANENT_DRAWER]: a wide portrait window now - * gets the rail, and gating on the dock would strip a panel it has today. Named - * `decideNotificationPanel` rather than `showsNotificationPanel` so it does not shadow - * [ScreenLayoutSpec.showsNotificationPanel] at the construction site. - * - * The [NavigationStyle.BOTTOM_BAR] term is not load-bearing — Compact is under 600dp, so a - * bottom-bar window cannot reach 1200dp — but it makes that an invariant the code enforces. + * Deliberately not keyed on [NavigationStyle]: a wide portrait window now gets the rail, and + * gating on the dock would strip a panel it has today. */ -internal fun decideNotificationPanel( - style: NavigationStyle, - windowWidthDp: Int, -): Boolean = - style != NavigationStyle.BOTTOM_BAR && - windowWidthDp >= NOTIFICATION_PANEL_MIN_WINDOW_DP +internal fun hasRoomForNotificationPanel(windowWidthDp: Int): Boolean = windowWidthDp >= NOTIFICATION_PANEL_MIN_WINDOW_DP /** * Centers a destination's content at [FeedContentMaxWidth]. The outer box paints the theme @@ -178,23 +169,15 @@ fun CappedScreenContent(content: @Composable () -> Unit) { } } -@OptIn(ExperimentalMaterial3WindowSizeClassApi::class) @Composable fun rememberScreenLayoutSpec(): ScreenLayoutSpec { - val widthSizeClass = calculateWindowSizeClass(getActivity()).widthSizeClass - val windowWidthDp = LocalConfiguration.current.screenWidthDp - return remember(widthSizeClass, windowWidthDp) { - val style = - when (widthSizeClass) { - WindowWidthSizeClass.Expanded -> NavigationStyle.PERMANENT_DRAWER - WindowWidthSizeClass.Medium -> NavigationStyle.NAV_RAIL - else -> NavigationStyle.BOTTOM_BAR - } + val configuration = LocalConfiguration.current + val windowWidthDp = configuration.screenWidthDp + val windowHeightDp = configuration.screenHeightDp + return remember(windowWidthDp, windowHeightDp) { ScreenLayoutSpec( - navigationStyle = style, - showsNotificationPanel = - style == NavigationStyle.PERMANENT_DRAWER && - windowWidthDp >= NOTIFICATION_PANEL_MIN_WINDOW_DP, + navigationStyle = decideNavigationStyle(windowWidthDp, windowHeightDp), + hasRoomForNotificationPanel = hasRoomForNotificationPanel(windowWidthDp), ) } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountSwitcherAndLeftDrawerLayout.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountSwitcherAndLeftDrawerLayout.kt index dbeddda760..1008e74dcb 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountSwitcherAndLeftDrawerLayout.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountSwitcherAndLeftDrawerLayout.kt @@ -138,8 +138,10 @@ fun AccountSwitcherAndLeftDrawerLayout( } /** - * Compact and Medium windows: the drawer slides in as a modal sheet. On Medium an - * [AppNavigationRail] sits at the left edge in place of the phone bottom bar. + * Every window that does not dock the drawer: the drawer slides in as a modal sheet. On the + * rail tier an [AppNavigationRail] sits at the left edge in place of the phone bottom bar and + * the shell becomes multi-pane — a wide portrait window rails and is still wide enough for + * the notification panel. */ @Composable private fun ModalDrawerShell( @@ -183,11 +185,12 @@ private fun ModalDrawerShell( }, content = { if (showRail) { - Row(Modifier.fillMaxSize()) { - AppNavigationRail(nav, accountViewModel) - VerticalDivider(thickness = DividerThickness) - CenterPane(Modifier.weight(1f), content) - } + MultiPaneShell( + accountViewModel = accountViewModel, + nav = nav, + leading = { AppNavigationRail(nav, accountViewModel) }, + content = content, + ) } else { content() } @@ -196,8 +199,62 @@ private fun ModalDrawerShell( } /** - * Expanded windows: the drawer is permanently docked on the left, the bottom bar disappears, - * and — when the window is wide enough — the notification feed docks on the right. + * The wide-window arrangement both shells render: a [leading] navigation pane, the centre + * content, and the notification feed when the window has room. Shared because a wide portrait + * window now rails rather than docks and is still wide enough for the panel — the two shells + * differ only in which navigation pane leads. + */ +@Composable +private fun MultiPaneShell( + accountViewModel: AccountViewModel, + nav: Nav, + leading: @Composable () -> Unit, + content: @Composable () -> Unit, +) { + Row(Modifier.fillMaxSize()) { + leading() + + VerticalDivider(thickness = DividerThickness) + + CenterPane(Modifier.weight(1f), content) + + NotificationSidePanelSlot(accountViewModel, nav) + } +} + +/** + * The docked notification feed, when there is room for it. The panel duplicates the + * Notifications screen, so it steps aside while the user is there. + * + * The back-stack entry is collected here rather than in the shells so windows too narrow for + * the panel never observe it, and so navigation churn recomposes this slot instead of the + * whole shell. The trade is that a rail-tier window wide enough for the panel ends up with a + * second collector beside [ModalDrawerShell]'s own — one extra subscriber on a shared flow, + * against a shell restart per navigation on every window that cannot show the panel. + * + * [Route] matching goes through `remember` because `hasRoute` resolves a serializer + * reflectively on every call. + */ +@Composable +private fun NotificationSidePanelSlot( + accountViewModel: AccountViewModel, + nav: Nav, +) { + if (!LocalScreenLayout.current.hasRoomForNotificationPanel) return + + val navBackStackEntry by nav.controller.currentBackStackEntryAsState() + val destination = navBackStackEntry?.destination + val onNotifications = remember(destination) { destination?.hasRoute() == true } + + if (!onNotifications) { + VerticalDivider(thickness = DividerThickness) + NotificationSidePanel(accountViewModel, nav) + } +} + +/** + * Wide, landscape, tall windows: the drawer is permanently docked on the left, the bottom bar + * disappears, and — when the window is wide enough — the notification feed docks on the right. */ @Composable private fun PermanentDrawerShell( @@ -206,24 +263,12 @@ private fun PermanentDrawerShell( openSheet: () -> Unit, content: @Composable () -> Unit, ) { - val navBackStackEntry by nav.controller.currentBackStackEntryAsState() - // The panel duplicates the Notifications screen, so it steps aside while the user is there. - val showPanel = - LocalScreenLayout.current.showsNotificationPanel && - navBackStackEntry?.destination?.hasRoute() != true - - Row(Modifier.fillMaxSize()) { - PermanentDrawerContent(nav, openSheet, accountViewModel) - - VerticalDivider(thickness = DividerThickness) - - CenterPane(Modifier.weight(1f), content) - - if (showPanel) { - VerticalDivider(thickness = DividerThickness) - NotificationSidePanel(accountViewModel, nav) - } - } + MultiPaneShell( + accountViewModel = accountViewModel, + nav = nav, + leading = { PermanentDrawerContent(nav, openSheet, accountViewModel) }, + content = content, + ) } /** diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayoutSpecTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayoutTest.kt similarity index 80% rename from amethyst/src/test/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayoutSpecTest.kt rename to amethyst/src/test/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayoutTest.kt index 5c846a9250..ac5c69857b 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayoutSpecTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayoutTest.kt @@ -21,6 +21,8 @@ package com.vitorpamplona.amethyst.ui.layouts import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue import org.junit.Test /** @@ -30,7 +32,7 @@ import org.junit.Test * * Sizes are window dp, width first. Every row of the spec's Behaviour table appears here. */ -class ScreenLayoutSpecTest { +class ScreenLayoutTest { private fun assertStyle( expected: NavigationStyle, widthDp: Int, @@ -86,19 +88,10 @@ class ScreenLayoutSpecTest { // ---- Notification panel ---- @Test - fun panelHiddenJustBelowTheThreshold() = assertEquals(false, decideNotificationPanel(NavigationStyle.PERMANENT_DRAWER, 1199)) + fun panelHiddenJustBelowTheThreshold() = assertFalse(hasRoomForNotificationPanel(1199)) @Test - fun panelShownExactlyAtTheThreshold() = assertEquals(true, decideNotificationPanel(NavigationStyle.PERMANENT_DRAWER, 1200)) - - @Test - fun panelShownAboveTheThreshold() = assertEquals(true, decideNotificationPanel(NavigationStyle.PERMANENT_DRAWER, 1201)) - - @Test - fun panelSurvivesTheCollapseToTheRail() = assertEquals(true, decideNotificationPanel(NavigationStyle.NAV_RAIL, 1200)) - - @Test - fun panelNeverAppearsOnTheBottomBar() = assertEquals(false, decideNotificationPanel(NavigationStyle.BOTTOM_BAR, 1200)) + fun panelShownExactlyAtTheThreshold() = assertTrue(hasRoomForNotificationPanel(1200)) /** * A landscape-short window: wide enough for the panel, too short for the dock. Rail plus @@ -106,8 +99,7 @@ class ScreenLayoutSpecTest { */ @Test fun shortLandscapeYieldsRailPlusPanel() { - val style = decideNavigationStyle(1200, 599) - assertEquals(NavigationStyle.NAV_RAIL, style) - assertEquals(true, decideNotificationPanel(style, 1200)) + assertStyle(NavigationStyle.NAV_RAIL, 1200, 599) + assertTrue(hasRoomForNotificationPanel(1200)) } } From 9e2859f719e038150347d274cc56aa0a08da97e8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 21:01:16 +0000 Subject: [PATCH 11/18] fix(quartz): stripe from the bucket, not from unrelated hash bits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit finding, and a real defect in the striped table two commits back. A striped hash table is only sound when the stripe is a function of the bucket. lockFor picked bits 16-19 of the hash while the bucket index used the low bits, so the 16 locks did not partition the table: two keys could share a bucket while holding different locks, and two writers would then read the same chain head and both publish over it. One insert silently disappears while entryCount counts both. The same window loses an overwrite, and loses entries through remove's chain rebuild. That is precisely the class of bug this work set out to remove from the copy-on-write version it replaced. Stripe now comes from `hash and (STRIPES - 1)`. Because STRIPES and every capacity are powers of two with STRIPES <= capacity, those are exactly the low bits of the bucket index, so same bucket implies same stripe at every size. It stays derived from the hash rather than the capacity, so a key keeps its stripe across a resize, which is what lets growTable exclude writers by taking all of them. INITIAL_CAPACITY is now defined as STRIPES so raising one cannot silently break the invariant. That definition also fixes a memory regression the audit caught: the table allocated 1024 slots eagerly, about 8 KB, per instance. LargeCache is not only the one big LocalCache — EphemeralRoom, RelaySession, PoolRequests and others build one per room, per connection and per subscription set, so a client holds hundreds that stay nearly empty. An empty instance goes from ~8 KB to ~970 bytes. Growth is geometric, so a table that does fill to 100k pays the same ~2n node rebuilds either way; re-measuring the shipped code confirms it (fill 16ms, overwrite 4ms, reads 1ms, 20 scans 16ms, mixed 86ms, 1 GC — unchanged within noise). The KDoc table is updated to those numbers. Adds LargeCacheStripingTest, which builds keys that share a bucket while differing in bits 16-19 and drives four workers at them behind a start barrier, with few enough buckets that chains grow long and each insert holds its lock for a while. It is documented for what it is: a stress test of the concurrent same-bucket path, not a deterministic reproducer — it did not fail against the broken striping in the runs attempted, which makes that race rare rather than absent. The fix rests on reading the stripe selection against the bucket index, not on a red test. Remaining known cost, noted in the KDoc rather than changed here: those ~970 bytes are nearly all the 16 PlatformLocks, two objects each. Folding them into one AtomicIntArray would reach ~250 bytes, but hand-rolling the spin wants its own review rather than a change on the way to merge. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HxQ1QuyzSkR38iFHbREjoS --- .../quartz/utils/cache/StripedHashMap.kt | 49 ++++- .../utils/cache/LargeCacheStripingTest.kt | 183 ++++++++++++++++++ 2 files changed, 224 insertions(+), 8 deletions(-) create mode 100644 quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheStripingTest.kt diff --git a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/StripedHashMap.kt b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/StripedHashMap.kt index ec9caba654..b50976cafa 100644 --- a/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/StripedHashMap.kt +++ b/quartz/src/linuxMain/kotlin/com/vitorpamplona/quartz/utils/cache/StripedHashMap.kt @@ -58,14 +58,23 @@ import kotlin.concurrent.atomics.ExperimentalAtomicApi * copy-on-write* n/a n/a n/a n/a n/a n/a n/a * HAMT + CAS 70 78 6 117 676 24 67MB * lock + HashMap 13 6 3 71 1197 36 51MB - * this 15 3 3 13 64 1 43MB + * this 16 4 1 16 86 1 41MB * ``` * * (*copy-on-write is off the scale: 20k entries alone took 18s to fill.) "mixed" is a * full fill with a whole-table scan every 1000 writes, which is the shape `LocalCache` * actually has — arriving events interleaved with feeds filtering the whole cache. - * Figures are one representative run of several; they were stable to within ~10%, - * except the HAMT's scan column, which wandered between 115ms and 190ms. + * Figures are one representative run of several; they were stable to within ~10%, except + * the HAMT's scan column, which wandered between 115ms and 190ms. This row was + * re-measured on the shipped code, after the stripe-selection fix and after + * [INITIAL_CAPACITY] dropped to [STRIPES] — neither moved it out of the noise. + * + * An *empty* instance costs ~970 bytes, nearly all of it the 16 [PlatformLock]s (two + * objects each). That is well above the ~50 bytes an empty map used to cost, and it is + * charged to every one of the hundreds of small caches a client holds. Folding the stripe + * locks into a single `AtomicIntArray` would take it to ~250 bytes and is the obvious next + * step if it ever shows up in a heap profile; it is left alone here because hand-rolling + * the spin is exactly the kind of change that wants its own review. * * ## Concurrency contract * @@ -111,10 +120,23 @@ internal class StripedHashMap { } /** - * Derived from the hash alone, never from the table size, so a key keeps the same - * stripe across a resize. + * **The stripe must be a function of the bucket**, or the locks do not partition the + * table and the whole design is unsound: two keys could share a bucket while holding + * different locks, so two writers would read the same chain head and both publish + * over it, silently dropping one insert. + * + * It is a function of the bucket here because [STRIPES] and every table capacity are + * powers of two with `STRIPES <= capacity`, so `hash and (STRIPES - 1)` is exactly the + * low bits of `hash and (capacity - 1)`. Same bucket therefore implies same stripe, at + * every size. Using the hash and not the capacity also keeps a key on one stripe + * across a resize, which is what lets [growTable] exclude writers by taking all of + * them. + * + * An earlier version took bits 16-19 instead. That is still resize-stable and still + * spreads well, which is why it looked right — but it is not derived from the bucket, + * so it broke the invariant above. */ - private fun lockFor(hash: Int) = locks[(hash ushr 16) and (STRIPES - 1)] + private fun lockFor(hash: Int) = locks[hash and (STRIPES - 1)] fun size(): Int = entryCount.load() @@ -301,8 +323,19 @@ internal class StripedHashMap { */ private const val STRIPES = 16 - /** Sized to carry a warm cache's first few thousand entries without a resize. */ - private const val INITIAL_CAPACITY = 1024 + /** + * Tied to [STRIPES] rather than chosen independently: the stripe is only a + * function of the bucket while `STRIPES <= capacity` (see [lockFor]), and defining + * it this way makes that impossible to break by raising [STRIPES] alone. + * + * Kept small on purpose. `LargeCache` is not only the one big `LocalCache` + * instance: `EphemeralRoom`, `RelaySession`, `PoolRequests` and friends each build + * one per room, per connection and per subscription set, so a client holds + * hundreds of them and most stay nearly empty. Starting at 1024 slots charged every + * one of those ~8 KB it would never use. Growth is geometric, so a table that does + * fill to 100k pays the same ~2n node rebuilds in total either way. + */ + private const val INITIAL_CAPACITY = STRIPES private const val MAX_CAPACITY = 1 shl 30 } diff --git a/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheStripingTest.kt b/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheStripingTest.kt new file mode 100644 index 0000000000..ec2b1901d4 --- /dev/null +++ b/quartz/src/linuxTest/kotlin/com/vitorpamplona/quartz/utils/cache/LargeCacheStripingTest.kt @@ -0,0 +1,183 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.utils.cache + +import kotlin.concurrent.atomics.AtomicInt +import kotlin.concurrent.atomics.ExperimentalAtomicApi +import kotlin.native.concurrent.ObsoleteWorkersApi +import kotlin.native.concurrent.TransferMode +import kotlin.native.concurrent.Worker +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotNull + +/** + * Stresses the one path the rest of the suite never reaches: **many writers inserting into + * the same bucket at the same time.** + * + * A striped table is only sound if the stripe is a function of the bucket. Derive the two + * from different parts of the hash and the locks stop partitioning the table: two keys can + * share a bucket while holding different locks, so two writers read the same chain head and + * both publish over it, and one insert vanishes. The first cut of [StripedHashMap] did + * exactly that — bucket from the low bits, stripe from bits 16-19 — and nothing here caught + * it, because every other suite uses small `Int` keys or a constant `hashCode`, which all + * collapse onto stripe 0 and serialise by accident. + * + * So the keys are built on purpose: groups of four sharing their low 16 bits, which puts a + * group in one bucket at any table size this reaches, while differing in bits 16-19 — the + * part the broken version striped on. Only [BUCKETS] buckets are used, so chains run to + * hundreds of nodes and each insert holds its lock for a while, which is what widens the + * window; with well-spread keys it is a few nanoseconds and nothing is observable. + * + * Being straight about what this is: a stress test, not a deterministic reproducer. It did + * not fail against the broken striping in the runs attempted, which says that race is rare + * rather than absent — the defect is a plain lost update, provable by reading + * [StripedHashMap]'s stripe selection against its bucket index, and was fixed on that + * basis. What this test is worth is being the only coverage of concurrent same-bucket + * inserts at all, and it would catch a coarser regression. + */ +@OptIn(ObsoleteWorkersApi::class, ExperimentalAtomicApi::class) +class LargeCacheStripingTest { + /** + * `hashOf` in [StripedHashMap] spreads with `h xor (h ushr 16)`, so to land on a final + * hash of `(member shl 16) or bucket` the raw hashCode has to be + * `(member shl 16) or (bucket xor member)`. The low 16 bits are then exactly `bucket`, + * shared by all four members of a group at every table size this test reaches. + * + * Only [BUCKETS] distinct buckets are used, so chains run to hundreds of nodes. That + * matters: a writer walks its chain *holding the stripe lock*, so a long chain is what + * makes the window wide enough for two writers on two different locks to overlap + * inside the same bucket. With well-spread keys the window is a few nanoseconds and + * the defect hides. + */ + private data class GroupedKey( + val group: Int, + val member: Int, + ) { + override fun hashCode(): Int { + val bucket = group and (BUCKETS - 1) + return (member shl 16) or (bucket xor member) + } + } + + /** Runs [job] on [WORKERS] threads released together, so they actually contend. */ + private fun inParallel(job: (workerId: Int) -> Unit) { + val ready = AtomicInt(0) + val go = AtomicInt(0) + val workers = List(WORKERS) { Worker.start() } + + val futures = + workers.mapIndexed { id, worker -> + worker.execute(TransferMode.SAFE, { Triple(job, id, ready to go) }) { (block, workerId, gates) -> + val (readyGate, goGate) = gates + readyGate.fetchAndAdd(1) + while (goGate.load() == 0) { } + block(workerId) + } + } + + while (ready.load() < WORKERS) { } + go.store(1) + + futures.forEach { it.result } + workers.forEach { it.requestTermination().result } + } + + @Test + fun concurrentInsertsAcrossSharedBucketsKeepEveryEntry() { + repeat(ROUNDS) { round -> insertRound(round) } + } + + private fun insertRound(round: Int) { + val cache = LargeCache() + + inParallel { worker -> + for (group in 0 until GROUPS) { + cache.put(GroupedKey(group, worker), group) + } + } + + val total = GROUPS * WORKERS + + val seen = mutableSetOf() + cache.forEach { key, _ -> seen.add(key) } + + assertEquals(total, seen.size, "round $round: iteration lost or duplicated entries in a shared bucket") + assertEquals(total, cache.size(), "round $round: size() disagrees with what the table holds") + + for (group in 0 until GROUPS) { + for (member in 0 until WORKERS) { + assertEquals( + group, + assertNotNull( + cache.get(GroupedKey(group, member)), + "round $round: entry ($group, $member) was dropped by a concurrent insert", + ), + ) + } + } + } + + @Test + fun concurrentCreateIfAbsentAcrossSharedBucketsReportsOneInsertEach() { + repeat(ROUNDS) { createIfAbsentRound() } + } + + private fun createIfAbsentRound() { + val cache = LargeCache() + val contended = GROUPS / 4 + + // Every worker races for the same keys this time, so a lost update shows up as a + // duplicate in the chain rather than a missing entry. + inParallel { _ -> + for (group in 0 until contended) { + for (member in 0 until WORKERS) { + cache.createIfAbsent(GroupedKey(group, member)) { group } + } + } + } + + val total = contended * WORKERS + val seen = mutableSetOf() + var visited = 0 + cache.forEach { key, _ -> + seen.add(key) + visited++ + } + + assertEquals(total, visited, "a key was inserted twice into the same chain") + assertEquals(total, seen.size) + assertEquals(total, cache.size()) + } + + companion object { + private const val WORKERS = 4 + + /** Large enough that the four workers overlap for essentially the whole run. */ + private const val GROUPS = 4_096 + + /** Few enough that chains grow long and every insert holds its lock for a while. */ + private const val BUCKETS = 64 + + /** Repeated on a fresh table, because only the *insert* path can lose a write. */ + private const val ROUNDS = 20 + } +} From 726f3e39c3af5baf1b1701adc2906f6bd8b0dc32 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Tue, 1 Sep 2026 17:41:30 -0400 Subject: [PATCH 12/18] fix(theme): pin the launch splash colour and drop the no-op night-mode writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two small independent fixes found while profiling the feed. Splash colour ------------- The system builds the launch splash from the manifest theme *before the process starts*, resolving it against the system light/dark configuration. The in-app ThemeType can be pinned to the opposite, so the splash flashed the wrong colour before the UI appeared: white before a dark UI for someone who pins DARK on a light system, black before a light UI for the reverse. No app-side code can fix that first frame — it is painted before onCreate runs. Pinning `windowSplashScreenBackground` to the brand colour already used for the status bar makes the splash read as intentional in every combination. Only the API 31+ splash attribute is set; `windowBackground` is deliberately left alone, so the window stays opaque (clearing it measured ~17% worse at frame P90, because a non-opaque window costs SurfaceFlinger the chance to skip the layers beneath it) and pre-31 behaviour is unchanged. Verified on an SM-T220 by recording a cold start and sampling frames: launcher -> purple splash (63,12,181 ≈ #3700B3) -> dark UI, with no white frame. Night-mode writes ----------------- `AmethystTheme` set `UiModeManager.nightMode` to force the device night mode for a pinned DARK/LIGHT theme. Changing it requires MODIFY_DAY_NIGHT_MODE, which the manifest does not declare, so the call silently no-ops for a normal app — while running a device-state write from inside composition on every recomposition of the theme. The pinned choice already takes effect through the colour scheme selected immediately below, which is what was actually doing the work. --- .../vitorpamplona/amethyst/ui/theme/Theme.kt | 26 +++++-------------- amethyst/src/main/res/values-night/themes.xml | 7 +++++ amethyst/src/main/res/values/themes.xml | 7 +++++ 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/theme/Theme.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/theme/Theme.kt index 3c74b51914..68838e57f6 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/theme/Theme.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/theme/Theme.kt @@ -21,8 +21,6 @@ package com.vitorpamplona.amethyst.ui.theme import android.app.Activity -import android.app.UiModeManager -import android.content.Context import androidx.compose.foundation.background import androidx.compose.foundation.border import androidx.compose.foundation.isSystemInDarkTheme @@ -49,7 +47,6 @@ import androidx.compose.ui.graphics.compositeOver import androidx.compose.ui.graphics.lerp import androidx.compose.ui.graphics.luminance import androidx.compose.ui.graphics.toArgb -import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.platform.LocalDensity import androidx.compose.ui.platform.LocalView import androidx.compose.ui.text.SpanStyle @@ -712,24 +709,15 @@ fun AmethystTheme( fontSize: FontSizeType = FontSizeType.NORMAL, content: @Composable () -> Unit, ) { - val context = LocalContext.current + // Deliberately no UiModeManager.nightMode write: changing the device night mode needs + // MODIFY_DAY_NIGHT_MODE, which this app does not declare, so the call silently no-ops — and it + // ran on every recomposition of the theme, writing device state from inside composition. The + // in-app choice is applied through the colour scheme below, which is what actually took effect. val darkTheme = when (prefTheme) { - ThemeType.DARK -> { - val uiManager = context.getSystemService(Context.UI_MODE_SERVICE) as UiModeManager - uiManager.nightMode = UiModeManager.MODE_NIGHT_YES - true - } - - ThemeType.LIGHT -> { - val uiManager = context.getSystemService(Context.UI_MODE_SERVICE) as UiModeManager - uiManager.nightMode = UiModeManager.MODE_NIGHT_NO - false - } - - else -> { - isSystemInDarkTheme() - } + ThemeType.DARK -> true + ThemeType.LIGHT -> false + else -> isSystemInDarkTheme() } val colors = remember(darkTheme, accentColor) { diff --git a/amethyst/src/main/res/values-night/themes.xml b/amethyst/src/main/res/values-night/themes.xml index 909b3e0a03..d436bd7ea1 100644 --- a/amethyst/src/main/res/values-night/themes.xml +++ b/amethyst/src/main/res/values-night/themes.xml @@ -2,6 +2,13 @@ diff --git a/amethyst/src/main/res/values/themes.xml b/amethyst/src/main/res/values/themes.xml index 0935ce7a7b..828ada16d5 100644 --- a/amethyst/src/main/res/values/themes.xml +++ b/amethyst/src/main/res/values/themes.xml @@ -2,6 +2,13 @@