Skip to content

[dotnet] fix template caching#17476

Merged
titusfortner merged 1 commit into
trunkfrom
dotnet_template
May 16, 2026
Merged

[dotnet] fix template caching#17476
titusfortner merged 1 commit into
trunkfrom
dotnet_template

Conversation

@titusfortner
Copy link
Copy Markdown
Member

💥 What does this PR do?

Code set up caching but didn't use it. Significantly faster devtools generation with it enabled

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): codex
    • What was generated: fix
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • Bug fix (backwards compatible)

@selenium-ci selenium-ci added the B-devtools Includes everything BiDi or Chrome DevTools related label May 16, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Enable template caching in dotnet devtools generator

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Enable template caching by storing compiled generators
• Significantly improves devtools generation performance
• Fixes unused caching infrastructure in TemplatesManager
Diagram
flowchart LR
  compile["Handlebars.Compile<br/>templateContents"]
  cache["m_templateGenerators<br/>cache"]
  return["Return generator"]
  compile -- "Store compiled<br/>generator" --> cache
  compile -- "Return to caller" --> return
Loading

Grey Divider

File Changes

1. third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs 🐞 Bug fix +3/-1

Store compiled templates in cache dictionary

• Store compiled Handlebars template generator in m_templateGenerators cache
• Assign compilation result to variable before returning
• Enables reuse of cached templates on subsequent calls

third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 16, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1)

Grey Divider


Action required

1. Edits under third_party/dotnet 📘 Rule violation ⚙ Maintainability
Description
This PR changes code inside third_party/, which is treated as read-only and should not be modified
in PRs. This can introduce untracked vendor drift and noisy diffs that are hard to maintain/audit.
Code

third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[R133-135]

+            var generator = Handlebars.Compile(templateContents);
+            m_templateGenerators[templatePath] = generator;
+            return generator;
Evidence
Compliance rule 9 prohibits modifying files under third_party/. The changed lines add caching
behavior by assigning to m_templateGenerators[templatePath] in
third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs.

AGENTS.md
third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[133-135]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR modifies code under `third_party/`, which is expected to be read-only.

## Issue Context
If this change is required, it should be applied in the non-vendored source location (or brought in via the appropriate vendor update process) rather than directly editing `third_party/`.

## Fix Focus Areas
- third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[133-135]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Unsafe cache dictionary writes 🐞 Bug ☼ Reliability
Description
TemplatesManager is registered as a DI singleton, and GetGeneratorForTemplate now mutates a
non-thread-safe Dictionary, so concurrent callers can crash or corrupt the cache state. This risk is
introduced by the new m_templateGenerators assignment.
Code

third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[R133-135]

+            var generator = Handlebars.Compile(templateContents);
+            m_templateGenerators[templatePath] = generator;
+            return generator;
Evidence
The cache is a plain Dictionary on a singleton service and is now mutated when compiling
templates, which is unsafe under concurrent access.

third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[17-17]
third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[34-40]
third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[132-135]
third_party/dotnet/devtools/src/generator/CodeGen/IServiceProviderExtensions.cs[18-33]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`TemplatesManager` is a singleton service and now writes into `Dictionary<string, Func<object,string>>` without synchronization. `Dictionary` is not safe for concurrent writers, so concurrent codegen runs (or multiple threads sharing the same DI container) can throw or corrupt internal state.

## Issue Context
- `TemplatesManager` is registered as a singleton in DI.
- `GetGeneratorForTemplate` compiles and then stores the generator into `m_templateGenerators`.

## Fix Focus Areas
- third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[17-17]
- third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[34-40]
- third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[132-135]
- third_party/dotnet/devtools/src/generator/CodeGen/IServiceProviderExtensions.cs[18-33]

## Suggested fix
Use a `ConcurrentDictionary` with `GetOrAdd` (ideally storing a `Lazy<Func<object,string>>` to ensure only one compilation happens), or guard both the read+compile+write section with a `lock` to ensure a single writer for a given key.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Unnormalized template cache key 🐞 Bug ➹ Performance
Description
GetGeneratorForTemplate resolves a full file path into targetTemplate for reading, but caches the
compiled generator under the original templatePath string, so equivalent paths (e.g., absolute vs
relative, or ./x.hbs vs x.hbs) will bypass the cache and recompile. This undermines the intended
performance improvement and can increase memory usage with duplicated cached entries.
Code

third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[R133-135]

+            var generator = Handlebars.Compile(templateContents);
+            m_templateGenerators[templatePath] = generator;
+            return generator;
Evidence
The method resolves a different path (targetTemplate) for IO than the one used for cache
operations (templatePath), and the PR change stores the generator under templatePath.

third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[34-46]
third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[48-55]
third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[133-135]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The cache key uses `templateSettings.TemplatePath` (`templatePath`) but the file is actually loaded from the resolved path `targetTemplate`. If callers supply different string representations of the same file path, the cache will miss and the template will be compiled multiple times.

## Issue Context
`targetTemplate` is computed by combining `Settings.TemplatesPath` with the `TemplatePath` if it is not rooted, but the cache lookup/store uses the original `templatePath` string.

## Fix Focus Areas
- third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[34-46]
- third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[48-55]
- third_party/dotnet/devtools/src/generator/CodeGen/TemplatesManager.cs[133-135]

## Suggested fix
Compute a normalized cache key from the resolved path, e.g.:
- resolve to `targetTemplate` as today
- set `var cacheKey = Path.GetFullPath(targetTemplate);`
- use `cacheKey` for both `TryGetValue` and the assignment
This ensures the cache maps 1:1 to the actual file that was compiled.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@titusfortner titusfortner merged commit 9ba0021 into trunk May 16, 2026
33 checks passed
@titusfortner titusfortner deleted the dotnet_template branch May 16, 2026 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-devtools Includes everything BiDi or Chrome DevTools related Compliance violation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants