Repository navigation
fix(view): prepare the draw.io import baseline once per rendering - #1694
Merged
Merged
Conversation
ImportDrawio laid the rendering out and routed every edge anew on each call to rebuild the same baseline. DrawioImporter prepares it once and Import diffs any file of the rendering against it; ImportDrawio delegates to it. TestDrawioRoundTrip reuses one importer per golden, so its trial imports no longer re-route the rendering, and runs its goldens in parallel. 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)".
|
HuiJun
marked this pull request as ready for review
October 29, 2026 19:02
Co-Authored-By: jason.han <hanhuijun@gmail.com>
6 tasks done
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
TestDrawioRoundTrip(added by #1679) took ~27 min under the race detector:ok internal/check/edit 1667son #1679's own Actions run, and ondevelopCircleCI'sGo race testsjob killed the package at its 30-minutego test -race -pgo=off -timeout 30mbudget (panic: test timed out after 30m0s,TestDrawioRoundTrip (29m35s)). The Actions shards allow 55 min per package (make test-shard), which is why only CircleCI went red.Cause.
Rendering.ImportDrawio(file, options)rebuilt its baseline on every call:drawioPreparelays the rendering out and routes every edge through libavoid (wazero), then the writer's document is serialised and parsed back, and only then isfilediffed against it. The baseline depends on the rendering and the options alone, not on the file, yet the test imported the same rendering 1 (untouched export) + up to E (trial imports ofdrawioEdge's pick, one per candidate edge) + 7 × 2 (each mutation's import and the re-import against the edited model) times per golden, and each import re-routed the whole rendering.Where the time went (
go test -race -run TestDrawioRoundTrip -cpuprofile,go tool pprof -top -cum, before the change, 8 vCPU):drawioImport75 % oftesting.tRunner's samples, all of itImportDrawio→drawioPrepare→routeEdges→route.routeAll→ wazerocallEngine.callWithStack→callEngine.cloneStack(14 % of all samples): wazero's exnreftry_tableentry handler clones the whole wasm stack on every C++try, and those allocations are what the GC (43.5 % of samples,scanObject/gcBgMarkWorker) and TSan (__tsan::MemoryAccessRangeT, 18.8 %) then chew on. Without the race detector the same profile is 42 % routing / 37.7 %cloneStack+ GC. Model loading, rendering,Applyand the semantic diff itself are milliseconds. Pooling wazero sessions does not help (measured), and wazeromainstill clones the stack there, so the lever is to route once, not to route faster.Change.
internal/ir/view:Rendering.DrawioImporter(options)prepares the baseline once (normalised rendering, writer, parsed baseline document, node/port index);DrawioImporter.Import(file)runs the matcher against it.ImportDrawionow delegates to the two, so the CLI and LSP callers are unchanged in behaviour. The matcher only readsnorm,w,base,nodesandports; its per-file state (framed,paired,matched,out) is still built perImport.internal/check/edit/drawio_test.go: one importer per golden shared by the untouched check, every trial import ofpickand every mutation's import, so a trial import is nowReadDrawio+ the semantic diff. The re-import against the edited model still prepares its own importer (a different rendering). The per-golden subtests run witht.Parallel(); nothing they touch is shared mutable state (drawioModelbuilds a fresh model/index/renderer per golden over the frozen shared library base,Apply's observer is mutex-guarded, the routing engine pools sessions under a mutex) and-racereports nothing.Contract preserved. Every golden under
internal/ir/view/testdata/*.drawio.goldenis still enumerated (the>= 30guard stays), all sevendrawioMutationsand their pickers are untouched, every assertion is the same code, and no timeout, tier or shard changed. A newTestDrawioImporterSharesOneBaselinepins that an importer answers the untouched export and a moved cell exactly as a freshImportDrawiodoes and refuses another view's page.How it was verified
Machine: 8 vCPU, go1.27.3, linux/amd64.
go test -race -count=1 ./internal/check/edit(whole package)TestDrawioRoundTripaloneok 1159.8s, 19m24s wall here; 27 min on #1679's Actions run; killed at 30 min on CircleCIok 136.4s, 2m17s wallgo test -race -count=1 -parallel 1 -run TestDrawioRoundTrip ./internal/check/edit(the importer alone, goldens serial — what a 1-vCPU box would see)ok 399.0s, 6m40s wallgo test -count=1 ./internal/check/edit(no race)ok 44.6s, 0m45s wallGates:
gofmt -l .empty,go vet ./...,go build ./...,make lint(staticcheck + gosec),go test -race -count=1 ./internal/check/edit ./internal/ir/view ./cmd/sysmlgreen, 0 data races.Checklist
make testandmake lintpass locally (the touched packages under-race; the full tier runs in CI)ImportDrawio's internalsLink to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/1ce956f62ffa43ccbe9697e4134ceb2e
Open in Devin Desktop: https://nasa-jpl-demo.devinenterprise.com/desktop/session/1ce956f62ffa43ccbe9697e4134ceb2e?variant=devin
Requested by: @HuiJun