Repository navigation
perf: speed up linting by memoizing ref target validation and reducing allocations - #274
Merged
Merged
Conversation
TristanSpeakEasy
added
performance
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Shadow auto-approve: would not auto-approve because issues were found.
View guided diff | Turn on auto-fix | Re-trigger cubic
Contributor
📊 Test Coverage ReportCurrent Statement Coverage: Coverage Change: 📈 +.1% (improved) Statement Coverage by PackageGo measures covered statements. Codecov measures fully covered lines, so its percentage can differ.
📋 Detailed Coverage by Function (click to expand)
Generated by GitHub Actions |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Performance optimization that memoizes $ref target validation and reflect.Type.Implements lookups and avoids allocations for nil schema fields, with tests confirming unchanged findings/exit codes and benchmarks showing substantial lint speedups.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Performance-only internal change: memoizes $ref target validation with fresh error copies, caches reflect.Type.Implements lookups, and skips nil schema child walks; tests pin equivalent findings and benchmarks show large lint speedups.
View guided diff | Turn on auto-fix | Re-trigger cubic
# Conflicts: # openapi/index.go
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.
Why
A recent public linter benchmark (write-up, repo) has Speakeasy CLI noticeably slower than Redocly on Stripe's spec. Profiling this library on the same inputs showed the lint rules themselves take about 20 ms. Nearly all of the time goes to parsing/validation and index building, and most of that is allocation and GC rather than real work.
Two specific hot spots:
BuildIndexre-ran full JSON Schema meta-validation of a$reftarget for every reference to it. Resolution already returns the same cached target object each time, so a heavily referenced component was validated hundreds of times with identical results.reflect.Type.Implementswas being recomputed for every node during unmarshalling and population.What changed
openapi.IndexmemoizesValidateresults per resolved$reftarget. Every reference still reports the target's errors, as fresh copies with their own document location, so output is unchanged.reflect.Type.Implementslookups in the marshaller are cached per type pair (interfaces.TypeImplements).oas3.walkSchemaskips building child locations for nil single-child fields (items,not,if, etc.), which were allocated and copied for every schema even when absent.No references, rules or validation are skipped.
Review notes
validateResolvedSchema/cloneValidationErrorsinopenapi/index.gois the part worth the closest look. It only caches when every error is a*validation.Error(so it can be copied faithfully), otherwise it just validates again.EitherValuefast path that populated the left value from a pointer to its core. It changed the source type handed to context-aware populators and was only worth about 2-3% CPU, so it was dropped after review.JSONSchema[T].Validateignores its options, so$reftargets are always meta-validated against the 3.1 dialect, even in 3.0 documents. I preserved that behaviour here.Testing
Benchmark inputs are the pinned Stripe specs from the benchmark repo. These are local runs on an M-series Mac under background load, interleaved base/candidate, 11 runs each, reporting the median.
openapi spec lintAbsolute times vary with machine load (an earlier, quieter run measured 1.50 s to 0.95 s and 1.68 s to 1.16 s), so the ratio is the useful number. On the earlier revision, library phases on Stripe (single file) went from index 590 ms to 285 ms and unmarshal/validation 1.01 s to 0.83 s, and the Speakeasy CLI built from source went 2.00 s to 1.56 s (single) and 2.28 s to 1.75 s (split).
spec lint -f jsonand text output matchmainon all 13 benchmark inputs (Stripe, Stripe split, DigitalOcean, GitHub, Cloudflare, AWS EC2, Azure, plus their bundled variants), as do the exit codes.TestBuildIndex_RepeatedSchemaRefValidation_Successpins per-reference error reporting (passes on bothmainand this branch),TestCloneValidationErrors_*cover the copy and non-copyable paths, andTestTypeImplements_MatchesReflect_Successchecks uncached and cached lookups against reflect using types unique to that test.mise cipassed locally on the latest commit (fmt-check, mod-check, golangci-lint, nilaway, examples-check, test, test-cli, build) with Go 1.26.0, matchinggo.work.Summary by cubic
Speeds up linting by memoizing
$reftarget validation results, cachingreflect.Type.Implementslookups, and reducing allocations during schema walking. Lint findings and exit codes are unchanged.Performance
openapi spec linton the Stripe single file drops from 1.50 s to 0.95 s; the 1,874-file split spec from 1.68 s to 1.16 s.Review notes
validateResolvedSchemareplays cached validation errors as fresh copies per reference; if any error is not a*validation.Error, it re-validates instead of caching.reflect.Type.Implementsresults are cached per type pair ininterfaces.TypeImplements.walkSchemaskips building child locations for nil single-child fields (items,not,if, and similar).getSequencedMapInterface,getSourceForPopulation).Written for commit 0a58a03. Summary will update on new commits.