perf(crawler): reuse JAXP factories in XmlTransformer#176
Merged
Conversation
Avoid per-document JAXP service discovery and reconfiguration in XmlTransformer. transform() previously ran DocumentBuilderFactory.newInstance() plus ~13 setFeature/setAttribute calls on every document, and getNodeList() built a fresh XPathFactory + XPath for every field rule of every document (the pre-existing per-thread XPathAPI cache was never actually used for node selection). - Configure the DocumentBuilderFactory once per thread (ThreadLocal) and only call newDocumentBuilder() per document; features/attributes are applied with identical keys, values, and order. - Route getNodeList through the existing per-thread XPathAPI cache via a new XPathAPI.selectNodeList(...), reusing the thread's XPath instead of rebuilding XPathFactory/XPath per rule. The namespace context is rebound and the expression recompiled on every call, because DefaultNamespaceContext is per-document and a compiled XPathExpression freezes prefix resolution at compile time; compiled expressions are therefore intentionally not cached across documents. Behavior is preserved (same parsed DOM, same node selection, same security features, same per-document namespace resolution) and all reuse is thread-confined. Add regression tests: sequential same-instance transforms (including a namespaced document that reuses a prefix bound to a different URI) and a 4-thread concurrent transform, locking cross-document namespace correctness and concurrent reuse.
The reused per-thread XPath in XmlTransformer kept the first document's DefaultNamespaceContext (which strongly references the parsed DOM) resting on it after the initial selectNodeList call, because the finally block only restored the previous context when it was non-null. On a fresh XPathAPI the previous context is null, so the first document's DOM stayed pinned for the lifetime of the per-thread cached XPathAPI. Reset the reused XPath to a shared, document-free empty NamespaceContext when there is no previous context to restore (setNamespaceContext rejects null). Correctness is unaffected because every call rebinds the context before evaluation. Add a regression test that exercises the first-call path and fails on the previous leaking finally block.
XmlTransformer is a DI singleton whose parser-config options are set only at bean wiring (before init) and never mutated at runtime, so the reused per-thread DocumentBuilderFactory only ever needs to be built once with the final configuration. Remove the config-version tracking, the holder wrapper, and invalidateDocumentBuilderFactory (which guarded a runtime reconfiguration that never happens), keeping just the per-thread ThreadLocal factory reuse. Setters now only assign their fields. Drop the two tests that exercised the removed runtime-reconfiguration path.
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.
Summary
XmlTransformerrebuilt heavyweight JAXP objects on every crawled document:transform()calledDocumentBuilderFactory.newInstance()(JAXP service discovery) plus ~13setFeature/setAttributecalls per document.getNodeList()built a brand-newXPathFactory+XPathfor every field rule of every document — the pre-existing per-threadXPathAPIcache was never actually used for node selection.This PR reuses those objects while keeping behavior and thread-safety intact.
Changes
createDocumentBuilderFactory()and held in a per-threadThreadLocal<DocumentBuilderFactory>initialized ininit().transform()now reuses the thread's configured factory and only callsnewDocumentBuilder()per document (a builder is stateful). All security features/attributes (secure processing, external general/parameter entity disabling, external DTD/schema access) and parser options are applied with identical keys, values, and order.getNodeList()now goes through the existing per-threadXPathAPIcache via a newXPathAPI.selectNodeList(node, expression, namespaceContext), which reuses the thread'sXPathinstead of rebuildingXPathFactory/XPathper rule.Design note: why compiled expressions are not cached across documents
A compiled
XPathExpressionfreezes namespace-prefix resolution at compile time.XmlTransformerresolves prefixes withDefaultNamespaceContext, which walks the current document's ownxmlns:*declarations — a genuinely per-document context. Caching a compiled expression by rule string alone would silently return wrong/empty results whenever two documents map the same prefix to different URIs. SoselectNodeListdeliberately rebinds the namespace context and recompiles per call; the win is eliminating the per-ruleXPathFactory.newInstance()and the per-document factory reconfiguration, not expression compilation.Thread-safety
All reuse is thread-confined: the
DocumentBuilderFactoryis a real per-ThreadThreadLocal, and the reusedXPathcomes from theXPathAPIcache keyed per crawler thread (the same pattern already used by the siblingHtmlXpathExtractor/HtmlExtractor). No JAXP object is shared across threads.Testing
XmlTransformerTest:test_transform_sequentialDocuments— two different documents through the same transformer instance (then the first again), asserting exact output each time (guards against factory/state leakage).test_transformNs_sequentialDocuments— a namespaced variant where the second document reuses the same prefix bound to a different namespace URI, specifically guarding the compile-time namespace-freeze pitfall.test_transformNs_concurrentDocuments— 20 tasks over a 4-thread pool, each with a uniquely-namespaced document on a shared transformer instance.fess-crawlermodule test suite passes (1981 tests, 0 failures);mvn formatter:format/license:format/javadoc:jarall clean.Relation to #175
Independent of #175 (the regex/hot-path quick-wins); this PR branches from
masterand touches a disjoint set of files.