OPENNLP-1883: Make stemmers thread-safe, add StemmerFactory and per-thread stem caching#1163
Open
krickert wants to merge 12 commits into
Open
OPENNLP-1883: Make stemmers thread-safe, add StemmerFactory and per-thread stem caching#1163krickert wants to merge 12 commits into
krickert wants to merge 12 commits into
Conversation
Contributor
Author
|
I'll flip off draft after the parent is merged, but reviews can start. |
…d SharingStemmer SnowballStemmer now routes each call to a per-thread generated engine via OwnerOrPerThreadState, the same pattern as the thread-safe *ME components, so one instance can be shared across threads without touching the generated Snowball code. StemmerFactory (api) captures stemmer configuration as an immutable, thread-safe seam for stateful engines; SharingStemmer adapts a factory into a shareable Stemmer for implementations that are not thread-safe themselves (e.g. PorterStemmer). NormalizationProfile.matchingAnalyzer() and TermAnalyzer are now safe to share across threads when stemming.
…unit tests MultiThreadedStemmerEval hammers shared SnowballStemmer instances for every algorithm, a SharingStemmer-wrapped PorterStemmer, and every NormalizationProfile matching analyzer from 8 threads, comparing each result against a single-threaded reference, mirroring MultiThreadedToolsEval. The unit tests now also cover all 21 algorithms under concurrency, the owner thread stemming concurrently with pool threads, virtual threads (a fresh thread per task), and Porter sharing.
…atch baseline Measures the thread-safe SnowballStemmer shared across threads and one per thread against a replica of the pre-patch plain-field implementation, at 1/8/32 threads. Results in BENCHMARKS.md: the OwnerOrPerThreadState overhead is within error bars up to 8 threads and only shows (~1.5x) at full 32-thread saturation.
…to-stem mappings Natural text is Zipf-distributed, so the same words are stemmed constantly. CachingStemmer wraps a StemmerFactory with a bounded per-thread LRU (default 1024 entries) following the same OwnerOrPerThreadState pattern as the *ME components: no cross-thread sharing, thread-safe regardless of the delegate. On the JMH zipf workload the cache is a ~34x throughput multiplier; on a cache-hostile uniform workload over 8x the cache capacity it still breaks even. Results and a corrected JMH invocation are documented in BENCHMARKS.md.
…ngStemmer Builder.stem(StemmerFactory) now wraps the factory in a CachingStemmer (default capacity), with a stem(StemmerFactory, int) overload for explicit cache sizing, and NormalizationProfile.matchingAnalyzer() builds through the factory again so profile analyzers get the per-thread stem cache for free. Analyzer results are unchanged; repeated words resolve from the cache instead of being re-stemmed.
Contributor
|
Hi @krickert - as mentioned on Slack I currently dont have the time for a manual review but I just let Fable do a comprehensive review on this PR. Here is the result: Main points:
Minor:
Human review will follow. |
…-indirection factory products SharingStemmer forwards stemAll to its per-thread delegate so a multi-output engine keeps its full result list through the wrapper, and all three thread-routing stemmers expose clearThreadLocalState(), mirroring the thread-safe ME components, so pooled threads can release their engine and cache instead of retaining them until the instance is collected. The caching javadoc and BENCHMARKS.md now state that the memoization is keyed to the physical thread and what that means on a virtual-thread-per-task executor. SnowballStemmerFactory products are now thread-confined stemmers driving their engine through a plain field, so the one-stemmer-per-thread pattern and the per-thread delegates inside the caching and sharing wrappers pay none of the shared-instance thread routing. The SnowballStemmer constructor validates algorithm and repeat like the factory, validation is IllegalArgumentException across the new surface, the LRU map is sized for its load factor so a full cache never rehashes, and StemmerFactory documents that it is a plain supplier, not one of the BaseToolFactory tool factories; its one-shot stem convenience method is gone. Tests cover the multi-output forwarding, the repeat pass-through with a witness word that stems differently at repeat one and two, the owner cache reset and worker-thread delegate release behind clearThreadLocalState, and the constructor validation.
Contributor
Author
|
Thanks. Addressed in b76eef4, with two deliberate keeps: Main points:
Minor:
|
rzo1
requested changes
Jul 11, 2026
rzo1
left a comment
rzo1
left a comment
Contributor
There was a problem hiding this comment.
Thank for the PR @krickert - I've reviewed the PR and left some comments.
My main suggestion: CachingStemmer and SharedStemmer could be generic over the wrapped Stemmer type. That would cut the current code duplication and let them wrap any concrete implementation including each other, e.g. a SharedStemmer inside a CachingStemmer, etc.
…g, cache controls The Stemmer and StemmerFactory interfaces state their contracts without implementation or threading claims, and null words throw IllegalArgumentException across the API (Snowball, factory products, caching and sharing wrappers, Porter), pinned by tests. SnowballStemmer now composes SharingStemmer so the per-thread routing pattern lives in one class. CachingStemmer gains clearCache() for the calling thread, interns cached stems through StringInterners, and both stateful classes document the container release path of the *ME components.
krickert
force-pushed
the
stemmer-factory
branch
from
bfeb0bd to
b76eef4
Compare
…erflow The cached stems are no longer routed through the JVM-wide interner: its default implementation holds strong references forever, so open-vocabulary input would grow without bound and defeat the documented per-thread cache cap. The LRU alone keeps the bound honest. The LRU table sizing is computed in double arithmetic because the int expression overflows to a negative capacity near Integer.MAX_VALUE; a test pins construction at the extreme. clearCache() documents that it initializes state for a thread that has not stemmed yet.
rzo1
approved these changes
Jul 12, 2026
| if (cached != null) { | ||
| return cached; | ||
| } | ||
| // toString() detaches the result from any buffer the delegate may reuse internally. The |
Contributor
There was a problem hiding this comment.
Think this comment can be dropped.
…d wrapper plumbing
Contributor
Author
|
Fixed the dedupe. I extracted a package-private |
…ate wrapper input Applies the review conventions from the OPENNLP-1869 review: em dashes and history framing leave the benchmark docs, rationale essays shrink to contract sentences, every override carries inheritDoc, and the stemmer wrappers validate their arguments at the boundary.
…m commentary Aligns null-argument validation with the project convention and removes remaining rationale commentary.
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
2 participants
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.
Thank you for contributing to Apache OpenNLP.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
For code changes:
For documentation related changes:
What is this PR for?
SnowballStemmerholds mutable per-call buffers in the generated engine, so sharing an instance across threads silently corrupts results — no exception, just wrong stems. With 3.0.0 unreleased, this aligns stemmers with the thread-safe*MEcomponents (OPENNLP-1816).Changes
SnowballStemmerroutes each call to a per-thread generated engine viaOwnerOrPerThreadState(same pattern as the*MEclasses); the generated Snowball code is untouched. One instance is now safe to share and is annotated@ThreadSafe.StemmerFactory(opennlp-api): an immutable, thread-safe capture of stemmer configuration, withSnowballStemmerFactoryandPorterStemmerFactoryimplementations and aSharingStemmeradapter for engines that are not thread-safe themselves (e.g.PorterStemmer).CachingStemmer: bounded per-thread LRU memoization of word-to-stem mappings. Natural text is Zipf-distributed, so most tokens are repeats; JMH shows ~34x throughput on a Zipf workload and break-even on a cache-hostile one.TermAnalyzer.Builder.stem(StemmerFactory)andNormalizationProfile.matchingAnalyzer()use it, so profile analyzers are shareable and cached by default.MultiThreadedStemmerEvalmirroringMultiThreadedToolsEval, and JMH benchmarks (SnowballStemmerBenchmark,CachingStemmerBenchmark) with results and run instructions in BENCHMARKS.md.Test plan
mvn teston opennlp-runtime: 1,363 tests pass (includes the 38 stemmer and 32 analyzer tests)mvn test -Peval-tests -Dtest=MultiThreadedStemmerEvalon opennlp-eval-tests: 3/3mvn clean test verify -Pjacoco -Pci: green