refactor(irverify): read declarations once per Verify run - #290
Merged
Conversation
checkReferentialIntegrity and checkDuplicateIDs each called ir.DeclaredIDs, so Verify walked the whole document twice to build the same slice. On a 500-operation document that was ~4ms of ~27ms, a sixth of the run. runWalkChecks reads the declarations once and hands them to every walking check. The four that do not need them take the parameter anyway: one signature is what lets walkChecks be a list, and the drift guard in walkchecks_test.go keys on the result types, which are unchanged. checkDuplicateIDs no longer walks for itself, so it passes on the flag of the walk its input rests on. Both the walkChecks contract and its own doc comment now say that rather than "its own walk".
DeclaredIDs said a node carrying an empty ID is "reported where the node's registry key is". That holds for the classes Document keys a map by and not for the two the same change added, which have no key: an operation or a service with an empty ID is reported by nothing. Filed as #289; the comment now states the gap instead of asserting it away. Three comments in validate_idrefs_test.go describe a set that has since grown. idRefSite named the classes it covers, TestValidate_DanglingTypedIDRef said it plants "channel, message or auth" references, and the location order above sortedIDRefPointers said "all three reference classes" while the literal below it gained six entries. The enumerations are replaced with the rule that derives them, so the next class to arrive cannot leave them wrong. identityClasses says per identity what holds it unique, and the TypeID entry named only checkRegistryKeys. checkDuplicateIDs holds it too, which is what TestCheckDuplicateIDs_TwoRegistryEntriesOnOneNodeID exists to show.
Two things a reader can only find by planting a mutation, so both are now written down beside the code. checkReferentialIntegrity folds decls.truncated into the flag it returns beside collectRefs' own. Neither of those walks prunes, so they truncate together and dropping either half is invisible to the suite. The fold stays for the reason the seed in runWalkChecks does, and now says so. The declarations zero value is not "nothing worth mentioning": it says the document declares no identity, which makes every OpID and ServiceID reference in it report as dangling. The suite does catch a check being handed one, but the type cannot, so the doc comment names it.
The declarations comment gave the saving as a fraction of Verify's cost, which is a figure that goes stale as the other checks change; the reason that cannot drift is that it was the same walk twice, and the measurement belongs in the pull request rather than the source. readDeclarations said it reads the identities "once for the run", which describes what runWalkChecks does with it and reads as though the function memoizes. It does not, and the tests call it per case.
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
Follow-up to #271, which added reference resolution and uniqueness checking for the two ID classes
Documentkeys no map by. Two things it left:irverify.Verifywalked the whole document twice tobuild the same list of declarations, and four comments describe a state that change moved past.
One walk instead of two.
checkReferentialIntegrityandcheckDuplicateIDseach calledir.DeclaredIDs, which is a full bounded walk.runWalkChecksnow reads the declarations once andhands them down. On a 500-operation synthetic document,
Verifydrops from ~27.3ms to ~23.4ms —about 14%, and the ~4.1ms a single
DeclaredIDscosts on that shape.pass.Validateis unchangedat ~12.4ms: its second walk is not the same redundancy, because the reference walk needs the
registries the declarations build, so it genuinely cannot run first.
The four checks that do not read the declarations take the parameter anyway. One signature is what
lets
walkChecksbe a list, and the drift guard inwalkchecks_test.gokeys on the result types,which are unchanged — every check still returns
([]Violation, bool)and still has to be listed.checkDuplicateIDsno longer walks for itself, so "each returns whether its own walk was cut short"stopped being true of it. It passes on the flag of the walk its input rests on, and both the
walkCheckscontract and its own doc comment now say that.The seed in
runWalkChecksis deliberately redundant.truncatedstarts atdecls.truncatedrather than
false. Both checks that read the declarations return that flag too, so seeding fromfalsereports the same thing today — planting that mutation leaves the suite green, and thecomment says so rather than leaving the line looking load-bearing. It is written this way because
the declaration walk now runs in
runWalkChecks, and a function that walks owning its own flag isthe whole point of #55: relying on a callee to hand back the flag for a walk this function performed
is the same dependence on a coincidence.
Comment corrections
ir.DeclaredIDsoverstated its own coverage. It said a node carrying an empty ID is "reportedwhere the node's registry key is". That holds for the classes
Documentkeys a map by —checkRegistryKeysreads an empty or disagreeing key — and not forOpIDandServiceID, the two#271 added, which have no key. Probed:
ID: ""ir/naming-absent)ID: ""ir/naming-absent)ID: ""under keyc/xir/channel-id-mismatchSkipping empty IDs is right and unchanged — nothing can reference one, and calling several of them
duplicates of each other would name the wrong defect. Only the justification was wrong, in the one
place a reader goes to find out whether the case is handled. Filed as #289; the comment now
states the gap.
Three comments in
validate_idrefs_test.godescribe a smaller set than the file now holds.idRefSitesaid the set is "every field whose type mentions ChannelID, MessageID or AuthID" andlisted the own-ID exclusions as "(Channel.ID, Message.ID, AuthScheme.ID)";
TestValidate_DanglingTypedIDRefsaid it plants "one dangling channel, message or auth reference";and the comment directly above
sortedIDRefPointerssaid "across all three reference classes" whilethe literal beneath it gained six entries in the same change. Each enumeration is replaced by the
rule that derives it, so the next class to arrive cannot leave them wrong — which is the property
the set itself already has and its description did not.
identityClassesomits a check it was written beside. The map says, per identity, what resolvesreferences to it and what holds it unique; the
TypeIDentry named onlycheckRegistryKeys.checkDuplicateIDsholds it too, which is exactly whatTestCheckDuplicateIDs_TwoRegistryEntriesOnOneNodeID, in the same file, exists to show. The threeentries reading "resolved and held as TypeID is" inherit the correction.
Test plan
Gate run in CI order, each exit code read directly rather than through a pipe:
gofmtclean,go vet0,golangci-lint0,go build0,./scripts/check-coverage.sh0 at 100%(4793/4793 statements).
Output equivalence. A refactor that preserves behaviour has to be shown to, not asserted. Both
revisions were run over the same 14 hand-built defective documents — duplicate op, triple op,
dangling op, empty op ID, empty op reference, duplicate service, dangling service, repeated PropID,
duplicate type ID, a document that truncates the walk, one that truncates and dangles, a nil
registry entry, a resource naming a ghost operation, and an empty document — plus every spec in
testdata, dumping every violation's code, path and message. The two dumps are byte-for-byteidentical, over 52 violations spanning 11 codes including all of
ir/duplicate-op-id,ir/duplicate-service-id,ir/duplicate-type-id,ir/dangling-op-ref,ir/dangling-service-refand
ir/walk-truncated.Mutation coverage. Every guard that held the behaviour before the signatures moved, plus every
line this change adds, was planted back:
checkReferentialIntegritydrops.WithDeclarationsTestCheckReferentialIntegrity_DanglingOpRef,…_DanglingServiceRef,…_DanglingOpRefWithNoOperationDeclaredcheckReferentialIntegritytrusts a truncated declaration walkTestCheckReferentialIntegrity_TruncatedWalkClaimsNoDeclarationscheckDuplicateIDsdrops thePropIDskipTestCheckDuplicateIDs_RepeatedPropIDIsClean,TestVerify_Corpus/component-reuse.yaml,TestVerify_EngineOutputcheckDuplicateIDsremoved fromwalkChecksTestVerify_ReportsDuplicateIDs,TestWalkChecks_NoWalkDropsItsTruncationFlagidentityClassesTestIdentityClasses_AreAllClassifiedreadDeclarationsdrops the truncation flagTestWalkChecks_EachReportsTruncation,TestCheckReferentialIntegrity_TruncatedWalkClaimsNoDeclarationscheckDuplicateIDshardcodestruncated = trueTestVerify_CleanDocHasNoViolationsand every other clean-document casecheckDuplicateIDshardcodestruncated = falseTestWalkChecks_EachReportsTruncationrunWalkCheckshands the checks a zerodeclarationsTestVerify_ReportsDuplicateIDs,TestVerify_ValidRawConfigIsCleanand othersrunWalkChecksseedstruncatedfromfalsecheckReferentialIntegritydropsdecls.truncatedfrom its foldmain, so this is inherited rather than introduced hereThe two surviving mutations are the two defence-in-depth lines. Both are kept for the reason #55
records — a walk's own flag must not depend on a sibling walk happening to trip the cap first — and
both now say at the line that they are redundant today, so neither reads as load-bearing.
Benchmarks are a scratch harness rather than a committed one; #78 is where committed benchmarks for
compile, marshal and validate belong, and adding one here would be a different change.
Related: #271, #289 (filed, not closed here).