Cache package-derived maps during .NET program generation - #1120
Cache package-derived maps during .NET program generation#1120corymhall wants to merge 2 commits into
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
Summary: - Record the repeated .NET program generation performance improvement. Rationale: - Include the required runtime changelog fragment for PR #1120. Tests: - not run (metadata-only change)
94fe3f5 to
4b9623a
Compare
corymhall
changed the title
Review Verdict: ApprovedMechanical hoist of existing per-call schema-scanning logic into a bounded, identity+shape-keyed cache; traced every read/write site of the cached maps and confirmed the shared references are never mutated after caching (only the per-generator namespaces clone is written). CI is fully green across the build/format/lint/codegen/conformance/integration matrix, and a targeted unit test covers the cache-invalidation-on-growth behavior. Automated low-risk assessment, not a substitute for human review. View session · Was this review helpful? Yes · No |
![eon-pulumi-agent[bot]](https://avatars.githubusercontent.com/in/4080224?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the diff for PR #1120 (commits 5037e1c and 4b9623a): pulumi-language-dotnet/codegen/gen_program.go, its test, and the changelog fragment.
What I checked
- Correctness of the new
packageContextForcache: cache-key matching (pointer identity + resource/function/type counts) correctly forces a rebuild when a partial package grows, matching the documented assumption thatpcl.PackageCachepackages only grow between calls (verified via a dedicated unit test,TestPackageContextCacheTracksPackageShape). - Whether the cached maps handed out to each
generatorare ever mutated after being cached. Onlynamespacesis deep-enough-cloned per generator (maps.Clone); the other four maps (compatibilities,tokenToModules,functionArgs,tokenPackages) are shared references. I traced every read/write site of all five fields ingen_program.goand confirmed the other four are read-only everywhere, and the one write site fornamespaces(g.namespaces[pkg] = info.Namespaces) only replaces a key in the per-call clone, never mutating a shared inner map — so the sharing is safe. - Security: no injection/authn/secrets/SSRF surface — this is pure in-memory Go map/slice manipulation over trusted schema structs with a bounded (16-entry) cache, so no DoS or cross-tenant leakage concern.
- AGENTS.md compliance: changelog fragment present and well-formed (
component: runtime,kind: Improvements), no submodule/generated-file/golden-file violations, formatting looks consistent withgofumptconventions. - CI: all required checks (build, format, lint, codegen-tests, conformance-tests, integration-tests across the ubuntu/macos/windows x .NET-version matrix) are green.
One pre-existing observation (not introduced by this PR, not blocking)
Left an inline note on a data race that already existed in the pre-PR code: ImportLanguages mutates the shared *schema.Package without any synchronization, and this PR's rebuild path still calls it outside the cache mutex. Since the PR only reduces how often this path runs (cache hits skip it entirely), it's a pre-existing hazard rather than a regression, so it isn't blocking here — but worth a follow-up if GenerateProgram can be invoked concurrently for the same shared package objects in practice.
Overall this is a well-scoped, mechanical hoist of existing per-call logic into a shared cache, with a solid before/after benchmark and a targeted regression test for the new invalidation behavior. This is an automated low-risk assessment, not a substitute for human review.
0607c3b to
8c9944f
Compare
Summary: - Record the repeated .NET program generation performance improvement. Rationale: - Include the required runtime changelog fragment for PR #1120. Tests: - not run (metadata-only change)
4b9623a to
663540f
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 663540f307
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tokenPackages: make(map[string]string), | ||
| } | ||
| entry := &packageContextCacheEntry{ | ||
| packages: slices.Clone(packages), |
There was a problem hiding this comment.
When the language host handles distinct GenerateProgram RPCs, main.go creates a fresh cached loader for each request, so these package pointers cannot produce cache hits across requests. Cloning them into this process-global cache nevertheless keeps the complete schema graphs from the last 16 requests reachable after their loaders close; for large providers or a long-lived host processing multiple projects, this can permanently add substantial memory and potentially cause OOMs. Scope the cache to the shared package cache/bulk-generation operation rather than retaining unrelated request packages globally.
Useful? React with 👍 / 👎.
![unblocked[bot]](https://avatars.githubusercontent.com/in/166219?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No issues found
About Unblocked
Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.
📖 Documentation — Learn more in our docs.
💬 Ask questions — Mention @unblocked to request a review or summary, or ask follow-up questions.
👍 Give feedback — React to comments with 👍 or 👎 to help us improve.
⚙️ Customize — Adjust settings in your preferences.
Summary: - Reuse immutable C# package lookup maps across program generation calls. - Invalidate cached contexts when a partial package gains schema members. - Bound retained contexts and preserve per-generator namespace mutation. Rationale: - Bulk documentation generation binds separate programs against shared package objects and previously rebuilt the same maps for every example. - This reduced AWS corpus generation time from 8.58s to 1.73s on top of the canonical-token optimization. Tests: - mise exec -- make format_language_host_check - mise exec -- make lint_language_host - mise exec -- make test_codegen
Summary: - Record the repeated .NET program generation performance improvement. Rationale: - Include the required runtime changelog fragment for PR #1120. Tests: - not run (metadata-only change)
663540f to
d26fae5
Compare
1 participant
Provider documentation generation binds each example separately but reuses one
pcl.PackageCache. This means thousands of programs can use the same package objects.GenerateProgramcurrently scans those packages and rebuilds the same C# lookup maps for every example.This change caches the namespace, compatibility, module, function argument, and token ownership maps. Cache entries use package identity and resource, function, and type counts, so the maps are rebuilt when a partial package grows. The cache is bounded, and each generator receives its own mutable top-level namespace map.
Measured across the complete AWS PCL corpus with the first PR applied:
Together, the two PRs change the primary and resource metrics from the original baseline as follows:
The generated files, diagnostics, and workload counts remained unchanged.
Validation:
mise exec -- make format_language_host_checkmise exec -- make lint_language_hostmise exec -- make test_codegen