rzo1 commented on PR #1163: URL: https://github.com/apache/opennlp/pull/1163#issuecomment-4938605047
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: - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/SharingStemmer.java`: `stemAll(CharSequence)` is not overridden, so the inherited default returns `List.of(stem(word))` and any multi-output delegate (the very case the new `stemAll` API anticipates) silently loses all but one stem form when wrapped. `CachingStemmer` forwards `stemAll` to its delegate correctly; `SharingStemmer` should do the same (`delegates.get().stemAll(word)`), and a test should cover this path. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/CachingStemmer.java` (also `SharingStemmer`, `SnowballStemmer`): none of the new stemmer classes exposes a release path to `OwnerOrPerThreadState.clearForCurrentThread()`, unlike the `clearThreadLocalState()` methods on `TokenizerME`, `SentenceDetectorME`, `POSTaggerME` and `NameFinderME` they claim to mirror. The owner-reset lambdas (e.g. `threadState.cache.clear()`) are therefore unreachable dead code, and pooled threads retain one engine plus an up-to-1024-entry cache per stemmer instance per thread until the instance is GC'd (with classloader-pinning risk on redeploy). This is aggravated by `NormalizationProfile.matchingAnalyzer()` building a fresh `CachingStemmer` per call, which leaves stale ThreadLocal entries and gets no cache reuse across calls. Either expose a clear/release method (and test it) or remove the lambdas. - `opennlp-api/src/main/java/opennlp/tools/stemmer/Stemmer.java`: the new default method `stemAll(CharSequence)` is speculative public API. No multi-output stemmer exists in the codebase (the javadoc cites Hunspell, which OpenNLP does not ship), the sole override in `CachingStemmer` is a pass-through, and the only callers are this PR's own tests. Multi-output normalization is already the contract of `opennlp.tools.lemmatizer.Lemmatizer`. Once shipped in `opennlp-api` at 3.0.0 this is frozen; suggest dropping it here and adding it under its own JIRA when a real multi-output stemmer lands. - `opennlp-api/src/main/java/opennlp/tools/stemmer/StemmerFactory.java`: the name collides conceptually with the sibling `BaseToolFactory`-based tool factories (notably `LemmatizerFactory`) while having an unrelated functional-interface contract and no javadoc distinguishing it, and it permanently claims the canonical name should stemmers ever join the `BaseToolFactory`/model-manifest mechanism. Consider a name outside that pattern (e.g. `StemmerProvider`/`StemmerSupplier`) or at least an explicit javadoc disclaimer. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/snowball/SnowballStemmer.java`: every `stem()` call now pays an `OwnerOrPerThreadState.get()` lookup, and the PR's own benchmarks show the previously supported one-instance-per-thread pattern regresses ~1.6x at saturation (4.77M vs 2.94M ops/s at 32 threads) with no opt-out back to the plain-field engine. Users who already confined a stemmer per thread gain nothing but inherit the cost; consider keeping a zero-indirection path. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/CachingStemmer.java`: per-thread state is keyed to the physical thread, so under `Executors.newVirtualThreadPerTaskExecutor()` (a pattern this PR's own tests exercise) every task re-allocates engine and cache and the advertised Zipf speedup never materializes. The 34x javadoc/BENCHMARKS.md claims only hold for reused platform threads; qualify the claims or offer a shareable bounded concurrent cache. Minor: - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/CachingStemmer.java`: `SnowballStemmerFactory` mints an already thread-safe `SnowballStemmer` as the per-thread delegate, so every cache miss pays the threadId/atomic lookup twice and each thread state carries a redundant second ThreadLocal plus an eagerly allocated engine. `TermAnalyzer.Builder.stem(StemmerFactory)` and `matchingAnalyzer()` take this nested path by default; a non-wrapped factory product would remove the double indirection. - `opennlp-api/src/main/java/opennlp/tools/stemmer/StemmerFactory.java`: the default convenience method `stem(CharSequence)` duplicates the `Stemmer.stem` signature on the factory type, and its own javadoc mainly warns against using it (mints a new stemmer per call). `newStemmer().stem(word)` is a trivial one-liner; suggest dropping the method. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/TermAnalyzer.java`: the `Builder.stem(StemmerFactory)` overload shares a name with `stem(Stemmer)` but silently adds caching/sharing semantics, and the pair is ambiguous for null literals or arguments typed as both interfaces. A distinct name (e.g. `stemCached`) would make the behavior explicit. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/CachingStemmer.java`: null-argument validation is inconsistent across the new API (`CachingStemmer`/`SharingStemmer` throw `IllegalArgumentException`, `SnowballStemmerFactory` uses `Objects.requireNonNull` and throws NPE). Pick one convention before this surface is frozen in 3.0.0. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/snowball/SnowballStemmerFactory.java`: the factory rejects `repeat <= 0` but the rewritten `SnowballStemmer(ALGORITHM, int)` constructor accepts any value unchecked; apply the same guard in the constructor. - `opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/stemmer/StemmerFactoryTest.java`: only the `repeat` validation is tested; nothing verifies that `new SnowballStemmerFactory(ENGLISH, 2).newStemmer()` actually passes `repeat` through (a pass-through typo would go undetected by the whole suite). - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/CachingStemmer.java`: the LRU map is built with `initialCapacity = capacity` at load factor 0.75, so it rehashes once when it passes ~0.75*capacity despite being size-bounded; use `(int) (capacity / 0.75f) + 1`. - `opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/util/normalizer/NormalizationProfilesTest.java`: `testMatchingAnalyzerIsThreadSafeForStemming` uses inline fully-qualified names (`java.util.concurrent.Executors` etc.) and `var`, inconsistent with the imports and explicit types used in the rest of the file and the sibling tests added by this PR. Human review will follow. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
