Phase 3 cleanup: embedded word list, nullable annotations, thread-safety docs - #37
Open
mattlorimor wants to merge 3 commits into
Open
Phase 3 cleanup: embedded word list, nullable annotations, thread-safety docs#37mattlorimor wants to merge 3 commits into
mattlorimor wants to merge 3 commits into
Conversation
Words.cs held 235,886 words as string literals across 235,908 lines, 5.75 MB of source the compiler parsed on every build to produce data that never changes. The words now live in words.txt, embedded in the test assembly and read once on first use, so tests that never touch the list do not pay for it at all. A clean build of the test project drops from about 1.01s to 0.70s. The more useful effect is that a 5.75 MB source file no longer sits in the tree for editors to load and diffs to step around; as data it is 2.38 MB. The list is unchanged. Extraction was checksummed: the SHA-256 of the words parsed out of the old array matches the SHA-256 of the lines in words.txt, both over 235,886 entries. Two entries are hyphenated names rather than plain words and no entry contains an escape or a newline, so newline-delimited text represents them exactly. Dictionary keeps its previous behavior, including returning the shared array instance rather than a copy when asked for everything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JH2NRDbhF5bwAsTAP7znb9
Nullable is enabled for the library only. The annotations ship in the package and give consumers null analysis against this API; the test and benchmark projects gain nothing from it and would only add noise, so they stay as they were. Repo-wide it produced 58 warnings, of which 7 were in the library. Most turned out to be modelling questions rather than annotation noise: - CuckooBloomFilter's buckets are byte[][][] whose innermost entries are null when a slot is empty, which the code already checks for. The type now says so. IndexOf null-checked an entry and then re-read the array slot instead of using the checked local, which the compiler could not connect; it uses the local now. - StableBloomFilter declared a private parameterless constructor that looked unused. It is not: NewUnstableBloomFilter builds instances with an object initializer, which needs it. Grepping for "new StableBloomFilter()" missed that, and deleting it broke the build -- worth recording, since the same reasoning nearly removed a live constructor. - Buckets64 assigns Data inside AllocateArray, which the compiler cannot see through; MemberNotNull states it. - Element.Data now defaults to an empty array rather than becoming nullable, so consumers reading TopK.Elements() do not have to null-check a value that is always set in practice. The one place a null-forgiving operator survives is the Cuckoo relocation loop, where an entry is read from a bucket the loop only enters when that bucket is full. Reaching a null there would have thrown before these annotations existed, so the operator records an existing invariant rather than asserting a new one. Thread safety is now documented rather than left to be inferred. Nothing in the library is synchronized, which matches the Go original and is a reasonable default, but it was written down nowhere. The subtle part is that Test is not safe to call concurrently with itself either, because filters reuse one HashAlgorithm instance and that type is not thread-safe; a reader-writer lock is therefore not enough. That is on IFilter for IntelliSense and in the README with an example. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JH2NRDbhF5bwAsTAP7znb9
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JH2NRDbhF5bwAsTAP7znb9
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The last three Phase 3 items. All non-breaking; folded into the unreleased 3.0.0 rather than shipped as a follow-up.
Word list moved out of C#
Words.csheld 235,886 words as string literals across 235,908 lines — 5.75 MB of source the compiler parsed on every build to produce data that never changes. It is now an embeddedwords.txt, read once on first use.A clean build of the test project drops from ~1.01s to ~0.70s. The more useful effect is that a 5.75 MB source file no longer sits in the tree for editors to load and diffs to step around.
The list is unchanged, verified by checksum rather than inspection: the SHA-256 of the words parsed out of the old array matches the SHA-256 of the lines in
words.txt, both over 235,886 entries. Two entries are hyphenated names and none contains an escape or newline, so newline-delimited text represents them exactly.Nullable reference types
Enabled for the library only. The annotations ship in the package and give consumers null analysis against this API; the test and benchmark projects gain nothing and would only add noise. Repo-wide it produced 58 warnings, of which 7 were in the library.
Most were modelling questions rather than annotation noise:
CuckooBloomFilter's buckets arebyte[][][]whose innermost entries are null when a slot is empty — which the code already null-checks. The type now says so.IndexOfalso null-checked an entry then re-read the array slot instead of using the checked local; it uses the local now.Element.Datadefaults to an empty array rather than becoming nullable, so consumers readingTopK.Elements()do not have to null-check a value that is always set in practice.Buckets64assignsDatainsideAllocateArray, which the compiler cannot see through;MemberNotNullstates it.One thing worth recording:
StableBloomFilterdeclared a private parameterless constructor that looked unused. Grepping fornew StableBloomFilter()found nothing, so I deleted it — and broke the build.NewUnstableBloomFilterbuilds instances with an object initializer, which requires it. Restored, with the members it populates marked at their declarations.The only null-forgiving operator that survives is in the Cuckoo relocation loop, where an entry is read from a bucket the loop only enters when that bucket is full. Reaching a null there would have thrown before these annotations existed, so it records an existing invariant rather than asserting a new one.
Thread safety documented
Nothing in this library is synchronized. That matches the Go original and is a defensible default, but it was written down nowhere, so consumers could not tell whether it was intentional.
The non-obvious part, now stated explicitly:
Testis not safe to call concurrently with itself, because filters reuse a singleHashAlgorithminstance and that type is not thread-safe. A reader-writer lock is therefore not sufficient — callers need an exclusive lock for every operation, or a filter per thread.On
IFilterfor IntelliSense, and in the README with an example.Verification
Build clean under
-warnaserror, 141 tests pass. All 12 complete README example programs re-extracted and compiled against the library after the edits.