perf(runtime): attribute and fix the 0.9.1 satisfaction, gRPC, migration and lint regressions - #742
Merged
Merged
Conversation
…ion and lint regressions Shared-default and shared-verdict tracing recorded every path of a nested shared value per read, deduplicated reads through a key string built per read, spelled a dotted path at every ancestor to ask whether a binding governs it, and every journal mark cloned the clock's waiter list. The trace now records a nested value once, compares paths in place, prefilters the binding question by the features a type's bindings start at (memoized on Model.bindingRoots), and the clock replaces its waiter list instead of editing it so a mark keeps the slice it saw. The migration writer reuses the buffers of closed blocks and indents from a table. The nested-usage subsetting memoizes declaredSubsettedNames on the runtime model and makes its deduplication and reachability sets on first use. The undeclared-signal lint walks the document once per analysis through kit.ScopedNodes instead of twice. The performance record's findings 5–8 carry the attribution, the six-run benchstat tables against v0.9.0 and the tree before this change, the profile frames, and what remains as the price of a rule. Co-Authored-By: jason.han <hanhuijun@gmail.com>
Contributor
Author
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # internal/translate/migrate/writer.go
Contributor
Author
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.
What and why
Closes out the "priced, not fixed" and "unattributed" findings of
docs/project/performance-release-0.9.1-vs-0.9.0.md(#728):Satisfy+46–56% / +64% allocs,GRPCVerifyConstraint+29%, migrateWriterSiblingBlocks+21%, and the single-threaded analysis cost. Each was profiled (-cpuprofile/-memprofile,pprof -top -cum) or bisected, the recomputation removed, and what remains attributed to the commit and rule that needs it. No semantics change: every fix is a memo, a fast path or an allocation removed, and no test, golden or corpus expectation was changed.Satisfaction tracing (
internal/exec/runtime) — the shared-default/shared-verdict trace of758c9c260/afbdb615dwas mostly recomputation around the record, not the record:observeReadcopied every path of a nested shared record into the trace per read → records the record pointer once (sharedRead.shared), expanded insharedPathsonly when a verdict is shared.sharedPathsdeduplicated through apathKeystring built per read and assembled each root-relative path twice →hasPathcompares slices; each path assembled once (sharedPath).bindingDeclaredForspelled a dotted path at every ancestor and looked each up inbindingsForFeature→Model.bindingRootsmemoizes per type the first segment of every binding end path; the path is spelled/looked up only where a binding starts at that feature (a binding matches only onend.Path == path, so the prefilter is exact).clock.waiters(slices.Clone[clockWaiter]was 289 MB of the run's heap — also in 0.9.0) →Clock.attach/detach/forgetFinishedreplace the slice instead of editing it in place, so a mark keeps the slice it saw.gRPC
VerifyConstraint— bisected to7bd51ece6(implicit nested-usage subsetting): +20% time / +17% bytes against its parent. The instance graph the response serializes now carries every nested usage's subsetted collection; the graphs before/after this PR are byte-identical and differ from 0.9.0 only in those collections, so the growth is the rule's output. Around it:declaredSubsettedNamesmemoized onModel.declaredSubsetted;appendUniqueInstancesandreachesNamesmake their sets on first use;reachesSubsettedlooks up the owner's values instead of building a by-name map.Migration writer —
101f3b7cdgrew the per-blockbuffer; the cost was a freshbufferper block andstrings.Repeatper line. The writer now reuses closed buffers (open/closewith a free list), indents from a table, and grows a line's builder once.Single-threaded analysis —
UndeclaredSignalPass.Runwas 19.8% ofAnalyseResolved, all inkit.WalkScoped→ast.Inspect, walked twice (gather sent signals, check triggers).kit.ScopedNodeskeeps the walk's result on the pass context for the document it analyzes (root looked up underResolver().Untrackedso no gather comes to depend on it); both passes iterate it. The lint's semantic queries were already journaled memos onsemantics.Model; no new cache there is warranted.Six-run interleaved
benchstat, v0.9.0 / before (17f8a80b5) / after:Satisfy/satellites=32Satisfy/satellites=128Satisfy/satellites=512SatisfygeomeanGRPCVerifyConstraintWriterSiblingBlocksAnalyseResolvedPriced, with commit and profile frames in findings 5–8 of the record: the per-read recording and per-ancestor
bindingRootedAtlookups of the tracing (+31% on the 32-satellite row, within noise at 128/512;758c9c260/afbdb615d); the subsetted collections the gRPC response now carries (+12% bytes;7bd51ece6); the lints and interface records on a resolved document (+14%).How it was verified
benchstatover six interleaved rounds of the four benchmark sets onv0.9.0,17f8a80b5and this tree; four-run pairs of7bd51ece6/101f3b7cdagainst their parents; CPU and heap profiles behind each finding (tables and commands in the record).gofmt -l .empty,go vet ./...,make lint,make build,go test -race ./...pass.OPENSYSML_REQUIRE_TRAINING_CORPUS=1 OPENSYSML_REQUIRE_PILOT_CORPORA=1(100/100 training clean; pilot ratchets unchanged at 56/58, 92/99, 56/56) and the SMT tests withOPENSYSML_REQUIRE_SMT=1 OPENSYSML_SMT=/usr/bin/z3pass.TestPathKeyDistinguishesDottedNames→TestHasPathDistinguishesDottedNames:pathKeyno longer exists, and the test asserts the same dotted-name property of its replacement.Checklist
make testandmake lintpass locallypathKeyunit test retargeted tohasPath; otherwise performance-only, covered by the existing suites and benchmarkschanges/unreleased/<slug>.<section>.md, not as an edit toCHANGELOG.mdmake docs-countsrun if a gate count moved — no gate count movedF4,K5) in the body, docs, or changelogLink to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/ef1f41df283a48fe8f40e04cfdcabd8d
Open in Devin Desktop: https://nasa-jpl-demo.devinenterprise.com/desktop/session/ef1f41df283a48fe8f40e04cfdcabd8d?variant=devin
Requested by: @HuiJun