perf(model): parse, index and validate a load's files on a pool of workers - #312
devin-ai-integration[bot] wants to merge 91 commits into
Conversation
Files loaded from the command line or by %load were joined into the transcript document, so a root-level import in one file served the others and two files declaring one root package were reported as duplicates. Each loaded file is now a workspace document under its own name, indexed with the others and analyzed on its own, as the editor and the corpus gates analyze it; the typed transcript stays one joined document. A differential test runs every multi-file directory of the fixtures and the OMG corpora through the command line and a workspace and asserts the same diagnostics. Co-Authored-By: jason.han <hanhuijun@gmail.com>
…pt alone Co-Authored-By: jason.han <hanhuijun@gmail.com>
…ing skill Co-Authored-By: jason.han <hanhuijun@gmail.com>
…kers A workspace opens a batch of documents in one step: the files are parsed and their scope trees built on workers, added to the one index in order, and the wildcard imports expanded once for the batch. Their diagnostics are computed on workers too, each with a context of its own over the index; before the pool starts, the metadata body scopes of the batch are linked to their owners, which resolving would otherwise write into the shared tree on first use. The results come back in the order asked, the same at any worker count. The document's own scope tree is the one the index holds, so a document is built once rather than twice. The REPL loads files through the batch; -workers and OPENSYSML_WORKERS set the count, one per CPU by default. The stress generator gains -split-planes. Co-Authored-By: jason.han <hanhuijun@gmail.com>
…al, and benchmark it -workers and OPENSYSML_WORKERS are checked from the command line over a model of several files, and BenchmarkValidateSplit loads the network split by plane on one worker and on one per CPU. Co-Authored-By: jason.han <hanhuijun@gmail.com>
Co-Authored-By: jason.han <hanhuijun@gmail.com>
…, and the gather it parallelizes Records the 200- and 1 600-satellite splits at one, two, four and eight workers beside the single file, the CPU and heap profiles that put the split's cost in the three workspace-wide audits, the allocation sites the pool does not help, and a benchmark of the per-document analysis alone. Co-Authored-By: jason.han <hanhuijun@gmail.com>
Co-Authored-By: jason.han <hanhuijun@gmail.com>
…er tools use Co-Authored-By: jason.han <hanhuijun@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
… load OPENSYSML_WORKERS now answers to its legacy SYSML_ name like the other variables, and -query, -render, -render-all and -compile resolve the run bounds before loading, as the other loading modes do. Co-Authored-By: jason.han <hanhuijun@gmail.com>
Co-Authored-By: jason.han <hanhuijun@gmail.com>
…lidation Co-Authored-By: jason.han <hanhuijun@gmail.com>
A loaded file is analyzed as a document of its own, so an error in it gates that file's deeper checks only. The blocker note on a clean prompt submission now skips diagnostics from loaded files, and a load's from the transcript. Co-Authored-By: jason.han <hanhuijun@gmail.com>
A load shares no document with the rest of the buffer, so nothing blocks it and it neither names nor forgets the error the transcript has already been told of. Co-Authored-By: jason.han <hanhuijun@gmail.com>
… interval A load still names no blocker, but when it leaves the transcript unblocked the recorded note is cleared, so the error is named again should a reload bring it back. Co-Authored-By: jason.han <hanhuijun@gmail.com>
OpenAll parses outside the lock, so a document another caller opened, edited, closed or removed meanwhile was overwritten at commit. The batch now records what each name held as it started and installs only where that still stands. Co-Authored-By: jason.han <hanhuijun@gmail.com>
…ocuments Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md
…lidation Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
… document is left A file reloaded with its enclosure left open is masked and its workspace document removed; with no scoped document left, symbolIndex returned before taking the file's previous declarations back out of the session index, so a qualified lookup kept answering with what the session no longer held. The empty-document path now drops every indexed document, as a reset does, and keeps the standard library. Co-Authored-By: jason.han <hanhuijun@gmail.com>
…lidation Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
…ocuments Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
…lidation Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
…ed cost Co-Authored-By: jason.han <hanhuijun@gmail.com>
Co-Authored-By: jason.han <hanhuijun@gmail.com>
…lidation Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
Co-Authored-By: jason.han <hanhuijun@gmail.com>
… a batch PrepareBatch linked the annotation bodies of the batch's documents only, but the workspace-wide gathers a batch's workers share read every workspace document. A batch asked for some documents therefore gathered the others' bodies unlinked: an unlinked body's attribute is filed under its bare name, where the identity gather made it collide with a declared id of a document that was asked for. PrepareBatch now links every workspace document, so a batch of any subset reports what one over every document does. Co-Authored-By: jason.han <hanhuijun@gmail.com>
…ocuments Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
…lidation Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| written := make(map[string]bool, len(files)) | ||
| for _, f := range files { | ||
| if err := os.Rename(filepath.Join(staging, f.Name), filepath.Join(dir, f.Name)); err != nil { | ||
| return stats, err |
There was a problem hiding this comment.
🔴 Concurrent edits are overwritten
When a generated file changes after replaceable checks it, os.Rename replaces the new content without revalidation. A concurrent editor save is lost.
Learn more
The overwrite check and replacement are separate filesystem operations. replaceable verifies the destination before staging, but another process can change that destination before os.Rename replaces it. The manifest digest then records generated content even though user content was overwritten.
Example: An editor saves plane000.sysml after line 76 verifies the old generated digest. The generator then renames its staged plane000.sysml over the save and reports success, instead of preserving the edit and refusing the generation.
Recommended fix: Serialize generations and edits with an output-directory lock where supported, or use a compare-and-swap replacement protocol. Immediately before replacement, atomically preserve the existing file under a private name, verify its digest, install the staged file only on a match, and restore the preserved file on mismatch or failure. Apply the same transaction discipline to concurrent generator processes and manifest publication.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The window exists: replaceable reads and digests each destination before staging, and the later os.Rename replaces whatever stands there at that moment, so a save landing on planeNNN.sysml between the two is overwritten and the final manifest records the generated digest.
What can close it is narrower than the note suggests. An editor writing into the output directory does not take any lock the generator could hold, so no single-process protocol makes the check and the replacement atomic against it: moving the standing file aside under a private name, digesting it and installing the staged file only on a match shrinks the window to the aside–install gap but cannot remove it, and a save that lands in that gap is still lost. A lock file in the output directory does serialize concurrent generator processes (the other party that would honour it), which is the case the manifest protocol was built to make safe.
Leaving this thread open for the maintainers' call: document that -split-planes expects sole ownership of its output directory while it runs (the current state, made explicit); add a lock file serializing concurrent generations; or add the aside-verify-install step per file as a best-effort narrowing on top of either.
…ocuments Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
Co-Authored-By: jason.han <hanhuijun@gmail.com>
…ocuments Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
…lidation Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
…ocuments Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
…lidation Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md # internal/core/model/workspace.go
…del does A batch's fresh models had no source lookup, so a filter on Comment::body, Documentation::body or TextualRepresentation::body was unevaluable in DiagnosticsAll and kept every candidate an editor's model hides. The batch now carries the workspace's read-only source lookup to the preparatory linker and every worker's model. Co-Authored-By: jason.han <hanhuijun@gmail.com>
…ocuments Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
…lidation Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
…ocuments Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
…lidation Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # README.md # docs/project/spec-compliance.md
…lidation Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # internal/core/edit/edit.go # internal/core/model/workspace.go
What and why
sysml -validate a.sysml b.sysml …,-satisfyand%loadnow open the files they are given as one batch and validate them on a pool of workers. This is the parallel batch pipeline ofdocs/project/large-model-scaling-design.md§5, built on #309 (one workspace document per loaded file), which this branch contains; merge #309 first, then this. Until #309 lands, the diff shown here includes its commits — the changes of this PR alone aregit diff feature/per-file-documents...perf/parallel-batch-validation(32 files).The branch is reconciled with the persistent semantic model and per-document gather cache of #316: the editor path (
Workspace.Diagnostics) analyzes on the workspace's persistent resolver, model and gathers throughpasses.AnalyzeShared; the batch path (Workspace.DiagnosticsAll) analyzes on private per-worker contexts throughpasses.AnalyzeInBatch, with one gather shared by the batch.The pipeline, in
internal/core/model/batch.go:symbols.Index.AddBuiltDocumenttakes a scope tree the caller built withsymbols.Build, so the batch builds its trees on the pool and the single writer only installs them. Wildcard imports are expanded once per batch rather than once per file added, which retires the quadratic per-file reindex cost the stress-test record noted.passes.AnalyzeInBatchin a context of its own; nothing takes a lock inside the resolver or the model. The one place resolution wrote to the shared scope tree — linking a metadata annotation body's owner (Scope.SetOwner) on first use — is done for all documents of the batch before the pool starts (resolve.(*Resolver).LinkMetadataBodiesviapasses.PrepareBatch, with the model-attached resolver analysis itself uses), so the workers only read it. To make that link computable outside a resolution, a metadata body scope records the declaration its annotation is written on (Scope.Annotated()), set by the builder.passes.Batch.Gatherscarries onepasses.Gathers(the per-document gather cache of feat(model): persistent semantic model per workspace, invalidated per document #316) to every context of the batch. The first context that runs a workspace-wide audit (OOSEMMethodPass,IdentityMetadataPass,MOSAPass) gathers every document's facts into it under its lock; every context afterwards reads the same union. The three passes are untouched — they already read throughContext.Gathers();AnalyzeInBatchsets the context's gathers to the batch's. The gathers are the batch's, not the workspace's persistent ones, because a private resolver records no dependencies and facts gathered by one could not be invalidated per dependency.DiagnosticsAllrecords the names it analyzed inWorkspace.batched;invalidateLockeddeletes thosediagCacheentries before asking the resolver what a change invalidates, andinvalidateAllLockedclears the set. ADiagnosticscall after an edit re-analyzes on the persistent model, with dependencies, as before.OpenAllparses outside the lock, so it records each input name's change count (Workspace.changes, bumped on every install and removal) before parsing and installs a document only where the count is still the one it reserved; a name opened, updated, closed or removed meanwhile — including one opened and removed again, absent both before and after — keeps its newer state (TestOpenAllKeepsAChangeMadeWhileItParsed).TestParallelBatchValidationMatchesSerialininternal/core/modelruns every fixture directory,examples/and the four OMG corpus roots at workers=1 and workers=GOMAXPROCS and asserts identical diagnostics (content and order), and identical to opening the files one by one.-workers Nonsysml, orOPENSYSML_WORKERS(legacySYSML_WORKERSviaenvvar.Lookup); the flag wins; defaultruntime.GOMAXPROCS(0); a value below one is rejected at startup before anything loads.OPENSYSML_JOBSis left to the runtime/check concurrency it already names. Documented insysml -help, the environment listing and the regenerated man page.Documentstays immutable and analysis state lives in each context; nothing in the pipeline holds a tree past its analysis except the workspace's owndocsmap, so releasing trees once interface records exist (§4) is a change to that map alone. Nothing is dropped in this PR.cmd/stress-model -split-planes <dir>writes one.sysmlper orbital plane pluslibrary.sysmlandconstellation.sysml(ground segment, cross-plane network), staged and recorded in.stress-model-filesso a later run removes only the plane files it owns.TestSatelliteNetworkSplitValidateschecks the split declares the single file's network, validates clean at one worker and at several, through the batch and through the persistent workspace, and that every satisfy assertion holds across files.Where the design and the implementation differ
-validatejoined the files into one<repl>document, so per-document parallelism had nothing to parallelize; feat(repl): analyze each loaded file as a workspace document of its own #309 is the prerequisite this branch is stacked on.Gathers.oosemOf2.7 s,identitiesOf0.55 s,mosaOf0.41 s), run by whichever context asks first over all 34 documents while the other workers wait at its lock; installing the scope trees and expanding wildcard imports before the pool (commitBatch, 1.1 s) is serial too. Gathering on the pool — each worker gathering its own document into the union before analysis starts, of which the per-documentGathers.Regatherthe editor path uses is the serial form — is the step left to ~5 s and is left as the follow-up; it touches the gather's locking and belongs in a change of its own.passes.Optionshas no option to disable the audit passes and none was added;BenchmarkAnalyzeSplitPerDocumentbuilds apasses.RegistryfromDefaultRegistry().Passes()minus the three and runs it over one index at one worker and one per CPU.DiagnosticsAllholds the workspace's write lock across the pool, asDiagnosticsalready does across one analysis. Batch parsing happens before the lock is taken.Measurements
Intel Xeon Platinum 8559C, 8 CPUs, 31 GiB, no swap, Go 1.25.0 linux/amd64,GOMAXPROCS=8;/usr/bin/time -v sysml -validate -memstats; one run per row; CPU = (user + sys) / wall. Models fromcmd/stress-model -planes 32 -satellites 50 -ground-stations 160(1 600 satellites; 299 137 elements, 18.4 MB in one file, 34 files split) and-planes 8 -satellites 25 -ground-stations 20(200 satellites, 10 files).The single file is unchanged from the figures taken before the gather was shared (18.5 s, 2.46 GB). Before it was shared, the 34-file split measured 129 s / 66.1 s / 38.6 s / 30.9 s at 1/2/4/8 workers, 39.1 GiB allocated, 1.98 → 7.24 GB peak RSS.
CPU profile, split 1 600, 8 workers (7.45 s wall, 28.6 s of samples,
-cpuprofile):passes.(*Gathers).gather3.5 s (12%) —oosemOf2.7 s,identitiesOf0.55 s,mosaOf0.41 s;NameResolutionPass.Run6.8 s (24%),W9CInheritedNameConflictPass.Run3.3 s,DiagramLayoutPass.Run1.3 s,TypeCheckPass.Run1.2 s;OpenAll1.1 s serial undercommitBatch(ExpandWildcardImports0.43 s);runtime.gcBgMarkWorker5.0 s (17.5%),runtime.scanobject5.1 s (18%). On one worker (19.6 s wall, 26.3 s of samples) the 34 analyses are 18.2 s of samples, 3.6 s of them the gather.Benchmarks (
internal/stressmodel,-benchtime 3x, four planes, six files):The per-document benchmark (audits left out) shows the pool's own speedup, 3.9× over six files, the largest file being about a quarter of the work. With the audits in,
BenchmarkValidateSplitwent 10.3 s → 5.77 s serial and 2.77 s → 1.67 s on eight workers when the gather became once per batch.Allocation follow-ups (listed, not implemented)
Parallelism leaves the allocation count unchanged (6.8 GiB, 90 M objects at any worker count). From the heap profile of the single-file 1 600-satellite run (62 M sampled objects, 4.3 GiB), by objects:
passes.(*w9cConflictChecker).specializespasses.contributionsOfsymbols.FQNOf(strings.Builder)symbols.(*Index).GetFQN, 18% viaconflictingBasesresolve.(*Resolver).specializationChainsemantics.(*Model).AllSupertypesparser.(*Parser).parseQualifiedNameRelaxedparser.(*Parser).parseBaseBy bytes the parser leads:
parseUsageand callees 25.6% of the 4.3 GiB (parseUsageValue7%),parseQualifiedNameRelaxed5.1%; thencontributionsOf6.6%,specializes4.9%,FQNOf4.4%. Each is to be measured on its own before it is changed; recorded indocs/internals/performance.md.One parse per load is spent twice, as before: the REPL parses each file to accept it (declared names, whether it closes its own text) and the workspace parses the same bytes again as the document. The 34 split files (17 MB) parse in 1.03 s serially — ~1 s of the one-worker 19.5 s, ~0.13 s of the eight-worker wall. Carrying the accepted tree into the workspace batch is a change to what
model.Inputowns, listed as a follow-up indocs/internals/performance.md.Edits to
passes/pass.go,passes/analyze.goandmodel/workspace.gopass.go: theBatchtype (Documents,Gathers) andContext.Batchfield.analyze.go:PrepareBatchandAnalyzeInBatchbesideAnalyzeShared; all three funnel through oneanalyze(ctx, root)that runs the registry, drops escalated warnings and sorts.workspace.go: aworkersfield withDefaultWorkers(); thebatchedset;invalidateLocked(names ...string)drops batch-computed diagnostics before asking the resolver;Open/Update/SetOnDiskandOpenAllinstall through oneinstallLocked(doc)— the library displacement,AddBuiltDocumentwith the document's already-built scope, and the library stand-in — thenExpandWildcardImportsonce per call;analyze(name, doc, batch)dispatches toAnalyzeInBatchwhen a batch is given andAnalyzeSharedon the persistent resolver, model and gathers otherwise. Serial incremental behavior is otherwise unchanged.symbols/index.go: adding a document the index already holds removes the old one without expanding wildcard imports (the publicRemoveDocumentstill does) and records the document as changed for the persistent resolver's invalidation; a batch of N reloads expands once, as N fresh files do (TestReplacingDocumentsExpandsOnceEqualToFreshBuild).core/edit/edit.go: the reindexer, which relied on that incidental expansion, now expands explicitly.How it was verified
gofmt -l .— nothing;go build ./...,go vet ./...— clean;python3 scripts/changelog.py check,make docs-check,make docs-counts(test count 8,444) — clean.go test ./...andgo test -race ./...— all packages ok.OPENSYSML_REQUIRE_TRAINING_CORPUS=1 OPENSYSML_REQUIRE_PILOT_CORPORA=1 go test -count=1 ./internal/core/model -run 'TestTrainingExamples|TestPilotCorpora'— ok;training_examples_expected.txtuntouched, no pilot ratchet moved.TestParallelBatchValidationMatchesSerial(fixtures,examples/, four OMG corpora; workers=1 vs N vs one-by-one),TestDiagnosticsAllAnswersInTheOrderAsked,TestOpenAllReplacesEarlierDocuments,TestOpenAllKeepsAChangeMadeWhileItParsed,TestWorkersSetting, theDiagnosticsAllsubset race test ininternal/core/model;TestWorkersFlagAndEnvironmentincmd/sysml;TestLinkMetadataBodies*ininternal/core/resolve;TestSatelliteNetworkSplitValidates,BenchmarkValidateSplit,BenchmarkAnalyzeSplitPerDocumentininternal/stressmodel.sysml -validateover the 200- and 1 600-satellite splits at workers 1/2/4/8 produces byte-identical stdout per model; the single-file output is byte-identical to develop's; every run reports no errors.Checklist
make testandmake lintpass locallychanges/unreleased/<slug>.<section>.md, not as an edit toCHANGELOG.mdmake docs-countsrun if a gate count moved (compliance rows need nothing: the census is counted at docs build)F4,K5) in the body, docs, or changelog