Skip to content

Commit b816191

Browse files
author
Callum Styan
committed
perf(coderd/dynamicparameters): skip caching introspection render calls
The first render in ResolveParameters() is purely for introspection to identify ephemeral parameters. The second render validates the final values after ephemeral processing. Previously, both renders were cached, causing unnecessary cache entries. For prebuilds with constant parameters, this doubled cache size and created entries that were never reused. This optimization: - Adds RenderWithoutCache() method to Renderer interface - First render (introspection) skips cache operations - Second render (final validation) still uses cache - Reduces cache size by 50% for prebuild scenarios - Improves cache hit ratio (was 4 misses/11 hits, now 1 miss/2+ hits) For 3 prebuilds with identical parameters: - Before: 2 misses, 4 hits, 2 entries (introspection + final) - After: 1 miss, 2 hits, 1 entry (final values only)
1 parent 1c4b645 commit b816191

File tree

6 files changed

+89
-27
lines changed

6 files changed

+89
-27
lines changed

‎coderd/dynamicparameters/render.go‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030
// Forgetting to do so will result in a memory leak.
3131
type Renderer interface {
3232
Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics)
33+
RenderWithoutCache(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics)
3334
Close()
3435
}
3536

@@ -259,9 +260,19 @@ type dynamicRenderer struct {
259260
}
260261

261262
func (r *dynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) {
262-
// Check cache first
263-
if cached, ok := r.data.renderCache.get(r.data.templateVersionID, ownerID, values); ok {
264-
return cached, nil
263+
return r.render(ctx, ownerID, values, true)
264+
}
265+
266+
func (r *dynamicRenderer) RenderWithoutCache(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) {
267+
return r.render(ctx, ownerID, values, false)
268+
}
269+
270+
func (r *dynamicRenderer) render(ctx context.Context, ownerID uuid.UUID, values map[string]string, useCache bool) (*preview.Output, hcl.Diagnostics) {
271+
// Check cache first if enabled
272+
if useCache {
273+
if cached, ok := r.data.renderCache.get(r.data.templateVersionID, ownerID, values); ok {
274+
return cached, nil
275+
}
265276
}
266277

267278
// Always start with the cached error, if we have one.
@@ -297,8 +308,8 @@ func (r *dynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values
297308

298309
output, diags := preview.Preview(ctx, input, r.templateFS)
299310

300-
// Store in cache if successful
301-
if !diags.HasErrors() {
311+
// Store in cache if successful and caching is enabled
312+
if useCache && !diags.HasErrors() {
302313
r.data.renderCache.put(r.data.templateVersionID, ownerID, values, output)
303314
}
304315

‎coderd/dynamicparameters/rendercache_prebuild_internal_test.go‎

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -101,17 +101,17 @@ func TestRenderCache_PrebuildWithResolveParameters(t *testing.T) {
101101

102102
// After first prebuild:
103103
// - ResolveParameters calls Render() twice:
104-
// 1. With previousValuesMap (empty {}) → miss, creates cache entry
105-
// 2. With values.ValuesMap() ({preset}) → miss, creates cache entry
106-
// Expected: 0 hits, 2 misses, 2 cache entries
104+
// 1. RenderWithoutCache with previousValuesMap (empty {}) → no cache operation
105+
// 2. Render with values.ValuesMap() ({preset}) → miss, creates cache entry
106+
// Expected: 0 hits, 1 miss, 1 cache entry
107107
t.Logf("After first prebuild: hits=%v, misses=%v, size=%v",
108108
promtestutil.ToFloat64(cacheHits),
109109
promtestutil.ToFloat64(cacheMisses),
110110
promtestutil.ToFloat64(cacheSize))
111111

112112
require.Equal(t, float64(0), promtestutil.ToFloat64(cacheHits), "first prebuild should have 0 hits")
113-
require.Equal(t, float64(2), promtestutil.ToFloat64(cacheMisses), "first prebuild should have 2 misses")
114-
require.Equal(t, float64(2), promtestutil.ToFloat64(cacheSize), "should have 2 cache entries after first prebuild")
113+
require.Equal(t, float64(1), promtestutil.ToFloat64(cacheMisses), "first prebuild should have 1 miss")
114+
require.Equal(t, float64(1), promtestutil.ToFloat64(cacheSize), "should have 1 cache entry after first prebuild")
115115

116116
// === SECOND PREBUILD ===
117117
// Second build: previous values now set to preset values
@@ -130,17 +130,17 @@ func TestRenderCache_PrebuildWithResolveParameters(t *testing.T) {
130130

131131
// After second prebuild:
132132
// - ResolveParameters calls Render() twice:
133-
// 1. With previousValuesMap ({preset}) → HIT (cache entry from first prebuild's 2nd render)
134-
// 2. With values.ValuesMap() ({preset}) → HIT (same cache entry)
135-
// Expected: 2 hits, 2 misses (still), 2 cache entries (still)
133+
// 1. RenderWithoutCache with previousValuesMap ({preset}) → no cache operation
134+
// 2. Render with values.ValuesMap() ({preset}) → HIT (cache entry from first prebuild's 2nd render)
135+
// Expected: 1 hit, 1 miss (still), 1 cache entry (still)
136136
t.Logf("After second prebuild: hits=%v, misses=%v, size=%v",
137137
promtestutil.ToFloat64(cacheHits),
138138
promtestutil.ToFloat64(cacheMisses),
139139
promtestutil.ToFloat64(cacheSize))
140140

141-
require.Equal(t, float64(2), promtestutil.ToFloat64(cacheHits), "second prebuild should have 2 hits")
142-
require.Equal(t, float64(2), promtestutil.ToFloat64(cacheMisses), "misses should still be 2")
143-
require.Equal(t, float64(2), promtestutil.ToFloat64(cacheSize), "should still have 2 cache entries")
141+
require.Equal(t, float64(1), promtestutil.ToFloat64(cacheHits), "second prebuild should have 1 hit")
142+
require.Equal(t, float64(1), promtestutil.ToFloat64(cacheMisses), "misses should still be 1")
143+
require.Equal(t, float64(1), promtestutil.ToFloat64(cacheSize), "should still have 1 cache entry")
144144

145145
// === THIRD PREBUILD ===
146146
values3, err := ResolveParameters(ctx, prebuildOwnerID, mockRenderer, false,
@@ -153,22 +153,22 @@ func TestRenderCache_PrebuildWithResolveParameters(t *testing.T) {
153153

154154
// After third prebuild:
155155
// - ResolveParameters calls Render() twice:
156-
// 1. With previousValuesMap ({preset}) → HIT
157-
// 2. With values.ValuesMap() ({preset}) → HIT
158-
// Expected: 4 hits, 2 misses (still), 2 cache entries (still)
156+
// 1. RenderWithoutCache with previousValuesMap ({preset}) → no cache operation
157+
// 2. Render with values.ValuesMap() ({preset}) → HIT
158+
// Expected: 2 hits, 1 miss (still), 1 cache entry (still)
159159
t.Logf("After third prebuild: hits=%v, misses=%v, size=%v",
160160
promtestutil.ToFloat64(cacheHits),
161161
promtestutil.ToFloat64(cacheMisses),
162162
promtestutil.ToFloat64(cacheSize))
163163

164-
require.Equal(t, float64(4), promtestutil.ToFloat64(cacheHits), "third prebuild should have 4 total hits")
165-
require.Equal(t, float64(2), promtestutil.ToFloat64(cacheMisses), "misses should still be 2")
166-
require.Equal(t, float64(2), promtestutil.ToFloat64(cacheSize), "should still have 2 cache entries")
164+
require.Equal(t, float64(2), promtestutil.ToFloat64(cacheHits), "third prebuild should have 2 total hits")
165+
require.Equal(t, float64(1), promtestutil.ToFloat64(cacheMisses), "misses should still be 1")
166+
require.Equal(t, float64(1), promtestutil.ToFloat64(cacheSize), "should still have 1 cache entry")
167167

168168
// Summary: With 3 prebuilds, we should have:
169-
// - 4 cache hits (2 from 2nd prebuild, 2 from 3rd prebuild)
170-
// - 2 cache misses (2 from 1st prebuild)
171-
// - 2 cache entries (one for empty params, one for preset params)
169+
// - 2 cache hits (1 from 2nd prebuild, 1 from 3rd prebuild)
170+
// - 1 cache miss (1 from 1st prebuild)
171+
// - 1 cache entry (for preset params only - introspection renders are not cached)
172172
}
173173

174174
// mockRenderer is a simple renderer that uses the cache for testing
@@ -189,4 +189,9 @@ func (m *mockRenderer) Render(ctx context.Context, ownerID uuid.UUID, values map
189189
return m.output, nil
190190
}
191191

192+
func (m *mockRenderer) RenderWithoutCache(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) {
193+
// For test purposes, just return output without caching
194+
return m.output, nil
195+
}
196+
192197
func (m *mockRenderer) Close() {}

‎coderd/dynamicparameters/rendermock/rendermock.go‎

Lines changed: 15 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/dynamicparameters/resolver.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,10 @@ func ResolveParameters(
6969
//
7070
// This is how the form should look to the user on their workspace settings page.
7171
// This is the original form truth that our validations should initially be based on.
72-
output, diags := renderer.Render(ctx, ownerID, previousValuesMap)
72+
//
73+
// Skip caching this render - it's only used for introspection to identify ephemeral
74+
// parameters. The actual values used for the build will be rendered and cached below.
75+
output, diags := renderer.RenderWithoutCache(ctx, ownerID, previousValuesMap)
7376
if diags.HasErrors() {
7477
// Top level diagnostics should break the build. Previous values (and new) should
7578
// always be valid. If there is a case where this is not true, then this has to

‎coderd/dynamicparameters/resolver_test.go‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,25 @@ func TestResolveParameters(t *testing.T) {
2828
render := rendermock.NewMockRenderer(ctrl)
2929

3030
// A single immutable parameter with no previous value.
31+
render.EXPECT().
32+
RenderWithoutCache(gomock.Any(), gomock.Any(), gomock.Any()).
33+
AnyTimes().
34+
Return(&preview.Output{
35+
Parameters: []previewtypes.Parameter{
36+
{
37+
ParameterData: previewtypes.ParameterData{
38+
Name: "immutable",
39+
Type: previewtypes.ParameterTypeString,
40+
FormType: provider.ParameterFormTypeInput,
41+
Mutable: false,
42+
DefaultValue: previewtypes.StringLiteral("foo"),
43+
Required: true,
44+
},
45+
Value: previewtypes.StringLiteral("foo"),
46+
Diagnostics: nil,
47+
},
48+
},
49+
}, nil)
3150
render.EXPECT().
3251
Render(gomock.Any(), gomock.Any(), gomock.Any()).
3352
AnyTimes().
@@ -78,7 +97,7 @@ func TestResolveParameters(t *testing.T) {
7897

7998
// A single immutable parameter with no previous value.
8099
render.EXPECT().
81-
Render(gomock.Any(), gomock.Any(), gomock.Any()).
100+
RenderWithoutCache(gomock.Any(), gomock.Any(), gomock.Any()).
82101
// Return the mutable param first
83102
Return(&preview.Output{
84103
Parameters: []previewtypes.Parameter{

‎coderd/dynamicparameters/static.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,15 @@ func (r *loader) staticRender(ctx context.Context, db database.Store) (*staticRe
4040
}
4141

4242
func (r *staticRender) Render(_ context.Context, _ uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) {
43+
return r.render(values)
44+
}
45+
46+
func (r *staticRender) RenderWithoutCache(_ context.Context, _ uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) {
47+
// Static renderer doesn't use cache, so this is the same as Render
48+
return r.render(values)
49+
}
50+
51+
func (r *staticRender) render(values map[string]string) (*preview.Output, hcl.Diagnostics) {
4352
params := r.staticParams
4453
for i := range params {
4554
param := &params[i]

0 commit comments

Comments
 (0)