Speed up structural testing: one read per file, no per-field regex or allocation - #26
Open
dionmcm wants to merge 4 commits into
Open
Speed up structural testing: one read per file, no per-field regex or allocation#26dionmcm wants to merge 4 commits into
dionmcm wants to merge 4 commits into
Conversation
RF2FileStructureTester.runTestForFile made three full passes over every file:
1. a readLine loop counting lines
2. a Scanner over a CRLF delimiter counting them again
3. a third open that read every line up to (totalLine - 1), purely to reach
the last one and look at its terminator
All three want the same facts, and one pass yields all of them: the line count
with readLine's semantics, the CRLF-separated token count Scanner produced, and
the final line's terminator.
Measured on the 594MB description file of a real edition, 4,174,475 lines:
pass 1 readLine count 1.33s
pass 2 Scanner CRLF 6.71s
pass 3 last-line reread 1.14s
---------------------------------
old total 9.19s
new single scan 3.26s 2.82x
Scanner was the most expensive of the three because it tokenises with the regex
engine - it took 6.71s to recount what readLine had counted in 1.33s.
Why this is worth doing now: with an engine that runs the assertion phase in 26s,
structural testing at 110s is the floor on a validation, so no engine change can
improve end-to-end time by more than about 9.5x while it stands. This removes
roughly a third of that phase.
Nothing about the report changes. The Scanner token count is reproduced exactly
rather than approximated - Scanner splits on each CRLF and returns the trailing
segment only when non-empty, so the count is the number of CRLF occurrences plus
one when anything follows the last of them, and adjacent CRLFs yield the empty
tokens that formula counts. RF2FileStructureTesterScanTest asserts the new scan
agrees with the old three passes, running the previous logic as a reference, over
twelve terminator shapes: all-CRLF, all-LF, mixed, empty, single line with and
without a terminator, an unterminated last line, adjacent terminators, CR-only,
a trailing empty line, and a header-only file.
One existing quirk is preserved deliberately: the last-line check only fires when
there are at least two lines, because reaching it required reading line
(totalLine - 1). A single-line file has never had its terminator checked. Fixing
that would report last-line terminators no release has been failed for before, so
it belongs in its own change if SI wants it.
Suite: 225 tests run, 0 failures, 142 errors, 21 skipped - the same 142 as clean
develop (Testcontainers with no Docker daemon on this host), 12 tests added.
… path
Structural testing is the floor on validation time, and after the previous commit
ColumnPatternTester was 92% of it - 70.3s of a 76.5s phase, measured by
instrumenting each tester. Four changes, none of which alters what is reported.
1. The regex engine is the per-field cost, and split() is not.
On the 1,039MB relationship file of a real edition (92,283,780 fields):
read only 3.88s
+ split("\t", -1) 3.37s (free, within noise)
+ regex per field 11.33s (+7.96s) <- the whole cost
+ the same checks as chars 3.80s (+0.43s)
So the seven patterns in this class now have hand-written equivalents and the
regex engine is the fallback for anything else. split() is left alone: it is not
the problem, and replacing it would mean changing PatternTest to take a
CharSequence with offsets, which is a signature change across every column test
for no measurable gain (index scanning measured 4.13s against 3.80s).
Equivalence is the whole risk here, so ColumnPatternFastPathTest holds each
replacement against its pattern over adversarial values and 200,000 random
strings drawn from an alphabet chosen to hit every boundary - digits, hex, '-',
ASCII spaces, line terminators, NBSP, U+2028. One case is worth naming:
String.isBlank() is NOT equivalent to NOT_BLANK (^(?=\s*\S).*$), because '.' does
not match a line terminator, so "a\nb" is non-blank to isBlank() and rejected by
the pattern. The replacement reproduces the pattern; a test pins that so nobody
simplifies it later.
2. errorArgs was built for every field, and read only on failure.
validate() opened by allocating a String[] and a String (lineNumber + "") for
every field of every line, while getErrorArgs() is read at exactly one call site,
immediately after validate() returns false. Now built on the failure path.
3. getPatternString() rebuilt a StringBuilder on every call, including the
success path, for a value fixed at construction. Built once.
4. Two more hot-path regexes removed: isNumericSctId ran SCTID_PATTERN per field
on the success path, isBlank ran ^$ where isEmpty() is exactly equivalent, and
DateTimeTest matched DATE_PATTERN directly instead of going through the shared
fast path - effectiveTime is one field on every row.
Also: both testers now submit largest-first. RF2 file sizes are extremely skewed
- on this edition the largest of 76 files is 20% of all bytes and the top eight
are 82% - so in directory order the small files finish while the giants are still
starting and the pool drains to two or three busy threads. ResourceProvider gains
getFileSize (default -1, so a provider that cannot answer keeps its old order)
and getFileNamesLargestFirst.
Measured end to end on the same host, same release, structural phase:
upstream baseline 110.3s
+ one pass for the line-terminator checks 86.4s
+ largest-first and the fast paths 71.0s
+ deferred allocations and the last hot-path regexes 38.5s 2.86x
Suite: 233 tests run, 0 failures, 142 errors, 21 skipped - the same 142 as clean
develop, 20 tests added.
What is left, for the record: the phase now runs at about 2.2 of 8 cores, so it is
bounded by the largest single file rather than by CPU. Splitting a file's lines
across threads would address that and is worth roughly another 2-3x, but it needs
per-range readers and merged reporting, which is a larger change than anything
here.
dionmcm
changed the title
Review feedback, and correct. Of the seven patterns this class uses, NOT_BLANK (^(?=\s*\S).*$) needed by far the most explanation for by far the least gain, so it now falls through to the regex engine like anything else without an equivalent. Measured, same branch and host, on the structural phase's column pass: with the NOT_BLANK fast path 31,792 ms without it 32,726 ms Under a second on a 32-second pass, which is inside run-to-run variance. In isolation over 2,246,130 real term values the hand-written version is 1.55x (0.439s to 0.284s) - real, but nothing beside the 8s that removing regex from the SCTID, effectiveTime and boolean columns saves on a single file, because those run on every field of every row while the lookahead fails fast on ordinary content. What goes with it: isNotBlankPattern, isAsciiSpace and isLineTerminator, and the agreement test for them. Three methods and a page of reasoning about why String.isBlank() is not equivalent - '.' rejects five line-terminator characters, so the pattern refuses a value isBlank() calls non-blank - and about which of those five can reach a term. None occurs anywhere in the 73 files of a real edition that were scanned for them. ColumnPatternFastPathTest keeps the isBlank case, rewritten as a warning rather than a check: anyone adding a fast path here has to reproduce that behaviour, and the measurement says not to bother.
Found by a reviewer asking what "adjacent CRLFs yield the empty tokens that
formula counts" meant. It was badly worded because it was wrong, and the wording
was hiding a real defect.
The claim was that Scanner's token count is the number of CRLF occurrences plus
one when anything follows the last of them. Checked against a real Scanner, that
is right for trailing and interior delimiters and wrong for leading ones:
input Scanner old formula
"\r\n" 0 1 <- would lose an error
"\r\n\r\n" 1 2 <- would lose an error
"\r\na" 1 2 <- would lose an error
"a\r\nb\r\n" 2 2
"a\r\n\r\nb\r\n" 3 3
The actual rule: splitting on every CRLF gives crlfCount + 1 segments, and
Scanner returns all of them EXCEPT an empty first segment and an empty last one.
Interior empty segments are kept, which is the only part the old wording got
right.
Consequence, had this shipped: a file whose first line is empty would report one
token against one line, so `tokens < totalLine` would be false and the
"total line is terminated with CR+LF" error would silently stop being raised. No
real release begins with a terminator, which is why the corpus never caught it -
and why the tests should have, but had no case starting with a delimiter.
Now derived from the segment rule, verified against a real Scanner over fourteen
shapes, and pinned by six new test cases covering leading, only, and
leading-then-content terminators. Reverting to the old formula fails three of them
with the counts above, so they discriminate.
Suite: 235 tests run, 0 failures, 142 errors, 21 skipped - the same 142 as clean
develop.
dionmcm
changed the title
Member
Author
|
Raised upstream as IHTSDO#75. |
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
1 participant
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 structural tests take about 110s of our overall full edition testing. This optimisation makes it 2.86x faster.
Measured on a SNOMED CT-AU edition, same host with 8 cores
Nothing about what gets reported changes.
1. Three reads per file where one would do
RF2FileStructureTester.runTestForFilemade three full passes:readLineloop counting linesScannerover a CRLF delimiter, counting againreadLinetototalLine - 1, to inspect the last line's terminatorOn the 594MB description file (4,174,475 lines): 1.33s + 6.71s + 1.14s = 9.19s, against 3.26s for one pass.
Scannerwas the most expensive because it tokenises with the regex engine.RF2FileStructureTesterScanTestruns the previous logic as a reference implementation and asserts agreement over eighteen terminator patterns.One existing quirk is preserved: the last-line check only fires with at least two lines, because reaching it required reading line
totalLine - 1. A single-line file has never had its terminator checked. Changing that would report terminators no release has been failed for, so I thought it belongs in its own change.2. The regex engine is the per-field cost;
splitis notOn the 1,039MB relationship file (92,283,780 fields):
Six of the seven patterns now have hand-written equivalents, with the regex engine as fallback.
splitis left alone: replacing it needsPatternTestto take aCharSequencewith offsets, a signature change across every column test, for no gain (index scanning measured 4.13s against 3.80s).NOT_BLANKdeliberately has no fast path. It needed the most explanation for the least gain: 31,792ms against 32,726ms on the column pass, inside run-to-run variance. In isolation over 2,246,130 real term values it is 1.55x, against the ~8s that removing regex from SCTID,effectiveTimeand boolean saves on a single file.ColumnPatternFastPathTestholds each remaining fast path against its pattern over adversarial values and 200,000 random strings per pattern. It keeps one case as a warning rather than a check:String.isBlank()is not equivalent toNOT_BLANK, because.does not match a line terminator, so the pattern rejects"a\nb".3. Per-field allocations
validate()opened by allocating aString[]and aString(lineNumber + "") for every field.getErrorArgs()is read at one call site, immediately aftervalidate()returns false. Now built there.getPatternString()rebuilt aStringBuilderon every call, including on the success path, for a value fixed at construction. Built once.Three further hot-path regexes removed:
isNumericSctIdranSCTID_PATTERNper field on the success path,isBlankran^$whereisEmpty()is equivalent, andDateTimeTestmatchedDATE_PATTERNdirectly instead of using the shared fast path.4. Largest-first scheduling
RF2 file sizes are skewed: the largest of 76 files is 20% of all bytes, the top eight are 82%. In directory order the small files finish while the giants are still starting, and the pool drains to two or three busy threads.
ResourceProvidergainsgetFileSize(default -1, so a provider that cannot answer keeps its order) andgetFileNamesLargestFirst.Verification
What remains
The phase now runs at about 2.2 of 8 cores, bounded by the largest single file rather than CPU. Splitting one file's lines across threads addresses that but introduces some complicated concurrency machinery for a 1.38x measured gain. Not worth it at present.