Repository navigation
perf(xbrl): stream standalone instances instead of building a DOM - #4814
stoyanov-x wants to merge 3 commits into
Conversation
|
Linking this to #4812, but deliberately not as #4812 describes two things: extraction throws Related, not fixed here: |
|
Streaming is the right direction, but I found two regressions against
Minor: two Please add both cases as tests; I'll take another look once they pass. A.L.V.I.S. |
Reported by @daniel3303 on daniel3303#4814, both reproduced and now pinned by tests. 1. TryReadFact rejected a non-numeric fact AFTER reading its content, and the caller then called Skip() - which swallowed the following sibling. In compact XML that sibling is the next fact, so a single "abc" value silently dropped its neighbour. With pretty-printed XML the swallowed node is whitespace, which is why my own 2000-fact fixture did not catch it. The method now reports whether it consumed the element, and the caller only skips what was left. 2. ReadElementContentAsString throws XmlException on an element with element children, e.g. an XBRL fraction item. That exception reached Parse's catch, which returns [], so ONE such item emptied the WHOLE document - worse than the LINQ-to-XML code, which only misread that one fact. Element text is now read by a helper that concatenates descendant text like XElement.Value and never throws. Also removes the orphaned <summary> left above TryMatchFiscalYearEnd. Known remaining difference, not fixed here: reader.Prefix is empty for a namespace bound as the default while GetPrefixOfNamespace returns its prefix when it is also bound with one. Flagged rather than guessed at. 125 parser and fiscal-year-end tests pass.
|
Thank you — both regressions are real, and both are fixed in 1. Swallowed neighbour fact. You are right that the reject happens after if (!TryReadFact(reader, contexts, units, out var fact, out var consumed))
{
if (!consumed)
reader.Skip();
continue;
}
2. Fraction item emptying the document. Confirmed, and worse than "misreads one fact" —
Minor items: the orphaned Prefix caveat, not fixed: 125 parser and fiscal-year-end tests pass. Ready for another look. Separately, and honestly: these two were caught in review, not by me, on a change whose entire premise was "same accept/reject rules, different mechanism". I have since verified that the local |
There was a problem hiding this comment.
Re-checked the latest commit. Both regressions are fixed correctly, and the consumed flag, ReadElementText across child elements and th
8000
e empty-element path all hold up. StandaloneXbrlParser has not changed on main since your base, so this is ready once the branch is updated with main and CI is green again.
One behaviour difference worth knowing: an unprefixed fact whose namespace is also bound to a prefix was accepted before (GetPrefixOfNamespace found the prefix) and is now rejected. I have not seen that in SEC instances, so it does not block.
A.L.V.I.S.
StandaloneXbrlParser materialised the whole instance as an XDocument, so peak
memory scaled with filing size. A standalone instance is the largest XML
artifact in this pipeline and a DOM expands multi-MB XML by roughly an order
of magnitude - enough that one oversized instance OOMs the extraction worker
(the XbrlFactsExtraction.Extract row in Errors).
Every pass now walks the document with XmlReader:
- contexts and units are lifted one at a time into a throwaway XElement via
ReadSubtree. They are few and small, and this keeps the existing
LINQ-to-XML logic byte-identical rather than re-deriving it;
- facts - the many, repeated part - are read straight off the reader and
never materialised.
Two passes are required because a fact may reference a context declared later
in the document; a single-pass resolver would silently drop those. There is a
test for exactly that, since no existing test covered it.
One trap worth naming, because I hit it: ReadElementContentAsString leaves the
reader on the node AFTER the element it consumed, so the obvious
`while (reader.Read())` loop skips whatever follows - on a fact-dense document
that is the next fact. On a 2000-fact fixture it returned 1000 facts, every
other one dropped, with no error. The loop now advances explicitly: fact
elements consume themselves and rejections are stepped over with Skip(). The
scale test pins both the count and the first/last values so a mis-advance
cannot pass by emitting a fact twice.
Accept/reject rules are unchanged, including the taxonomy-prefix contract:
XmlReader.Prefix is the alias used on the element, which matches
GetPrefixOfNamespace for any document that declares it in scope, and an
unprefixed default-namespace element is rejected in both.
123 parser and fiscal-year-end tests pass, 37 of them pre-existing.
Scope note: InlineXbrlParser is untouched. It parses HTML with AngleSharp, not
XML, and has no streaming API - that is a rewrite, not a swap, and belongs in
its own change.
CI's format check caught this; the local build does not run csharpier, so it only surfaced on the push. dotnet csharpier check . is clean now.
Reported by @daniel3303 on daniel3303#4814, both reproduced and now pinned by tests. 1. TryReadFact rejected a non-numeric fact AFTER reading its content, and the caller then called Skip() - which swallowed the following sibling. In compact XML that sibling is the next fact, so a single "abc" value silently dropped its neighbour. With pretty-printed XML the swallowed node is whitespace, which is why my own 2000-fact fixture did not catch it. The method now reports whether it consumed the element, and the caller only skips what was left. 2. ReadElementContentAsString throws XmlException on an element with element children, e.g. an XBRL fraction item. That exception reached Parse's catch, which returns [], so ONE such item emptied the WHOLE document - worse than the LINQ-to-XML code, which only misread that one fact. Element text is now read by a helper that concatenates descendant text like XElement.Value and never throws. Also removes the orphaned <summary> left above TryMatchFiscalYearEnd. Known remaining difference, not fixed here: reader.Prefix is empty for a namespace bound as the default while GetPrefixOfNamespace returns its prefix when it is also bound with one. Flagged rather than guessed at. 125 parser and fiscal-year-end tests pass.
334ce36 to
13c13ea
Compare
StandaloneXbrlParsermaterialised the whole instance as anXDocument, so peak memory scaled with filing size. A standalone instance is the largest XML artifact in this pipeline, and a DOM expands multi-MB XML by roughly an order of magnitude — enough that one oversized instance OOMs the extraction worker.Every pass now walks the document with
XmlReader:XElementviaReadSubtree. They are few and small, and this keeps the existing LINQ-to-XML logic byte-identical instead of re-deriving it.Two passes are required because a fact may reference a context declared later in the document; a single-pass resolver would silently drop those. There is a test for exactly that, since no existing test covered it.
One trap worth naming
ReadElementContentAsStringleaves the reader on the node after the element it consumed, so the obviouswhile (reader.Read())loop skips whatever follows — on a fact-dense document, the next fact. On a 2000-fact fixture it returned 1000 facts, every other one dropped, with no error. The loop now advances explicitly: fact elements consume themselves, rejections are stepped over withSkip(). The scale test pins the count and the first/last values, so a mis-advance cannot pass by emitting a fact twice.Accept/reject rules are unchanged, including the taxonomy-prefix contract:
XmlReader.Prefixis the alias used on the element, which matchesGetPrefixOfNamespacefor any document that declares it in scope, and an unprefixed default-namespace element is rejected in both.Tests
123parser and fiscal-year-end tests pass —37of them pre-existing and unmodified, plus two new ones (StandaloneXbrlParserStreamingTests): fact-before-its-context resolution, and a 2000-fact instance.Scope
InlineXbrlParseris deliberately untouched. It parses HTML with AngleSharp, not XML, and has no streaming API — that is a rewrite, not a swap, and belongs in its own change. This PR does not claim to fix every OOM in the pipeline: of the 7OutOfMemoryExceptionrows on our instance, only 1 is inXbrlFactsExtraction, and 4 are inDocumentScraper.ProcessFiling.