Skip to content

Commit d801eea

Browse files
committed
perf: replace defensive deepcopy with deep_freeze immutability
expand_language_extensions was deep-copying the template 4-10 times per invocation for defensive isolation (cache put, cache get, no-LE path, LE path). For large templates this is measurable overhead. Replace with a one-time deep_freeze that converts dicts to MappingProxyType and lists to tuples. Frozen templates are truly immutable — any mutation attempt raises TypeError immediately instead of silently corrupting shared state. Callers that need mutable copies use deep_thaw (not copy.deepcopy which cannot pickle MappingProxyType on Python 3.13). Savings: - No-LE fast path: 4 deepcopies -> 1 deep_freeze (same O(n) cost) - Cache hit: 2 deepcopies -> 0 (return cached frozen result directly) - Cold LE path: 3 deepcopies -> 2 deep_freezes (expanded + original) - Cache put: 2 deepcopies -> 0 (frozen values are safe to share) Also: - Fixed isinstance(x, dict) check in build_context to accept Mapping so frozen MappingProxyType templates pass the type check. - Updated callers (build_context, package_context, artifact_exporter, language_extensions_packaging, wrapper) to use deep_thaw instead of copy.deepcopy when they need mutable copies. - Updated tests to verify immutability contract (TypeError on mutation) instead of asserting independent mutable copies.
1 parent 19bd98e commit d801eea

10 files changed

Lines changed: 100 additions & 59 deletions

File tree

samcli/commands/build/build_context.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import pathlib
88
import shutil
99
from collections import Counter
10+
from collections.abc import Mapping
1011
from typing import Any, Dict, List, Optional, Tuple
1112

1213
import click
@@ -40,6 +41,7 @@
4041
sanitize_resource_key_for_mapping,
4142
substitute_loop_variable,
4243
)
44+
from samcli.lib.cfn_language_extensions.utils import deep_thaw
4345
from samcli.lib.intrinsic_resolver.intrinsics_symbol_table import IntrinsicsSymbolTable
4446
from samcli.lib.providers.provider import LayerVersion, ResourcesToBuildCollector, Stack
4547
from samcli.lib.providers.sam_api_provider import SamApiProvider
@@ -442,17 +444,16 @@ def _get_template_for_output(self, stack: Stack, modified_template: Dict, artifa
442444
Dict
443445
The template to write to disk
444446
"""
445-
import copy
446447

447448
# If no original template, use the modified (expanded) template
448449
# Check if original_template_dict exists and is a dict (not a Mock or other type)
449450
original_template_dict = getattr(stack, "original_template_dict", None)
450-
if not isinstance(original_template_dict, dict):
451+
if not isinstance(original_template_dict, (dict, Mapping)):
451452
return modified_template
452453

453454
# Use the original template (with Fn::ForEach intact)
454455
# We need to update artifact paths in the Fn::ForEach constructs
455-
original_template = copy.deepcopy(original_template_dict)
456+
original_template = deep_thaw(original_template_dict)
456457

457458
# Update artifact paths in the original template
458459
self._update_original_template_paths(original_template, modified_template, stack)

samcli/commands/package/package_context.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ def run(self):
171171
def _export(self, template_path, use_json):
172172
from samcli.commands.validate.lib.exceptions import InvalidSamDocumentException
173173
from samcli.lib.cfn_language_extensions.sam_integration import expand_language_extensions
174+
from samcli.lib.cfn_language_extensions.utils import deep_thaw
174175

175176
# Read the original template
176177
with open(template_path, "r", encoding="utf-8") as f:
@@ -192,7 +193,7 @@ def _export(self, template_path, use_json):
192193

193194
uses_language_extensions = result.had_language_extensions
194195
dynamic_properties = result.dynamic_artifact_properties
195-
template_dict_for_export = result.expanded_template
196+
template_dict_for_export = deep_thaw(result.expanded_template)
196197

197198
# Create Template with the (possibly expanded) template
198199
template = Template(

samcli/lib/cfn_language_extensions/sam_integration.py

Lines changed: 19 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,12 @@
2727
ResolutionMode,
2828
TemplateProcessingContext,
2929
)
30-
from samcli.lib.cfn_language_extensions.utils import FOREACH_PREFIX, FOREACH_REQUIRED_ELEMENTS, is_foreach_key
30+
from samcli.lib.cfn_language_extensions.utils import (
31+
FOREACH_PREFIX,
32+
FOREACH_REQUIRED_ELEMENTS,
33+
deep_freeze,
34+
is_foreach_key,
35+
)
3136

3237
LOG = logging.getLogger(__name__)
3338

@@ -77,6 +82,11 @@ class LanguageExtensionResult:
7782
This dataclass carries all Phase 1 outputs so that callers don't need
7883
to re-derive them.
7984
85+
All template fields are deeply frozen (dicts as ``MappingProxyType``,
86+
lists as ``tuple``). Callers that need to mutate must use
87+
``deep_thaw()`` from ``samcli.lib.cfn_language_extensions.utils``
88+
(not ``copy.deepcopy`` which cannot pickle ``MappingProxyType``).
89+
8090
Attributes
8191
----------
8292
expanded_template : Dict[str, Any]
@@ -552,31 +562,18 @@ def expand_language_extensions(
552562
cached = _expansion_cache.get(cache_key)
553563
if cached is not None:
554564
LOG.debug("Cache hit for template expansion: %s", template_path)
555-
return LanguageExtensionResult(
556-
expanded_template=copy.deepcopy(cached.expanded_template),
557-
original_template=copy.deepcopy(cached.original_template),
558-
dynamic_artifact_properties=list(cached.dynamic_artifact_properties),
559-
had_language_extensions=cached.had_language_extensions,
560-
)
565+
return cached
561566

562567
if not check_using_language_extension(template):
568+
frozen = deep_freeze(template)
563569
result = LanguageExtensionResult(
564-
expanded_template=copy.deepcopy(template),
565-
original_template=copy.deepcopy(template),
570+
expanded_template=frozen,
571+
original_template=frozen,
566572
dynamic_artifact_properties=[],
567573
had_language_extensions=False,
568574
)
569575
if cache_key is not None:
570-
# Store a deep copy so callers can't poison the cache
571-
_cache_put(
572-
cache_key,
573-
LanguageExtensionResult(
574-
expanded_template=copy.deepcopy(result.expanded_template),
575-
original_template=copy.deepcopy(result.original_template),
576-
dynamic_artifact_properties=list(result.dynamic_artifact_properties),
577-
had_language_extensions=result.had_language_extensions,
578-
),
579-
)
576+
_cache_put(cache_key, result)
580577
return result
581578

582579
LOG.debug("Expanding CloudFormation Language Extensions (Phase 1)")
@@ -599,8 +596,8 @@ def expand_language_extensions(
599596
LOG.debug("Successfully expanded CloudFormation Language Extensions")
600597

601598
result = LanguageExtensionResult(
602-
expanded_template=expanded_template,
603-
original_template=copy.deepcopy(template),
599+
expanded_template=deep_freeze(expanded_template),
600+
original_template=deep_freeze(template),
604601
dynamic_artifact_properties=dynamic_properties,
605602
had_language_extensions=True,
606603
)
@@ -611,16 +608,7 @@ def expand_language_extensions(
611608
EventTracker.track_event(EventName.USED_FEATURE.value, UsedFeature.CFN_LANGUAGE_EXTENSIONS.value)
612609

613610
if cache_key is not None:
614-
# Store a deep copy so callers can't poison the cache
615-
_cache_put(
616-
cache_key,
617-
LanguageExtensionResult(
618-
expanded_template=copy.deepcopy(result.expanded_template),
619-
original_template=copy.deepcopy(result.original_template),
620-
dynamic_artifact_properties=list(result.dynamic_artifact_properties),
621-
had_language_extensions=result.had_language_extensions,
622-
),
623-
)
611+
_cache_put(cache_key, result)
624612

625613
return result
626614

samcli/lib/cfn_language_extensions/utils.py

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@
55
for working with templates that may contain Fn::ForEach blocks.
66
"""
77

8-
from typing import Dict, Iterator, Tuple
8+
from types import MappingProxyType
9+
from typing import Any, Dict, Iterator, Tuple
910

1011
FOREACH_PREFIX = "Fn::ForEach::"
1112

@@ -110,3 +111,42 @@ def iter_regular_resources(template_dict: Dict) -> Iterator[Tuple[str, Dict]]:
110111
for key, value in template_dict.get("Resources", {}).items():
111112
if not is_foreach_key(key) and isinstance(value, dict):
112113
yield key, value
114+
115+
116+
def deep_freeze(obj: Any) -> Any:
117+
"""Recursively make a template structure immutable.
118+
119+
- dicts become ``MappingProxyType`` (read-only view)
120+
- lists become ``tuple`` (immutable sequence)
121+
- primitives (str, int, float, bool, None) pass through unchanged
122+
123+
Any caller that tries to mutate a frozen template gets an immediate
124+
``TypeError`` instead of silently corrupting shared state. Callers
125+
that need a mutable copy should use ``deep_thaw()`` (not
126+
``copy.deepcopy`` which cannot pickle ``MappingProxyType``).
127+
128+
Cost is O(n) — same as one ``copy.deepcopy`` — but you pay it once
129+
at creation time and then never need defensive copies again.
130+
"""
131+
if isinstance(obj, MappingProxyType):
132+
return obj # already frozen
133+
if isinstance(obj, dict):
134+
return MappingProxyType({k: deep_freeze(v) for k, v in obj.items()})
135+
if isinstance(obj, list):
136+
return tuple(deep_freeze(item) for item in obj)
137+
return obj
138+
139+
140+
def deep_thaw(obj: Any) -> Any:
141+
"""Recursively convert a frozen template back to mutable dicts and lists.
142+
143+
Inverse of ``deep_freeze``. Use this instead of ``copy.deepcopy``
144+
on frozen templates (``MappingProxyType`` is not picklable).
145+
"""
146+
if isinstance(obj, MappingProxyType):
147+
return {k: deep_thaw(v) for k, v in obj.items()}
148+
if isinstance(obj, dict):
149+
return {k: deep_thaw(v) for k, v in obj.items()}
150+
if isinstance(obj, (list, tuple)):
151+
return [deep_thaw(item) for item in obj]
152+
return obj

samcli/lib/package/artifact_exporter.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
from samcli.commands._utils.experimental import ExperimentalFlag, is_experimental_enabled
2424
from samcli.commands.package import exceptions
2525
from samcli.commands.validate.lib.exceptions import InvalidSamDocumentException
26-
from samcli.lib.cfn_language_extensions.utils import iter_regular_resources
26+
from samcli.lib.cfn_language_extensions.utils import deep_thaw, iter_regular_resources
2727
from samcli.lib.package.code_signer import CodeSigner
2828
from samcli.lib.package.local_files_utils import get_uploaded_s3_object_name, mktempfile
2929
from samcli.lib.package.packageable_resources import (
@@ -222,7 +222,7 @@ def do_export(self, resource_id, resource_dict, parent_dir):
222222
normalize_template=True,
223223
normalize_parameters=True,
224224
parent_stack_id=resource_id,
225-
template_str=yaml_dump(result.expanded_template),
225+
template_str=yaml_dump(deep_thaw(result.expanded_template)),
226226
parameter_values=parameter_values,
227227
)
228228
template.template_dir = child_template_dir

samcli/lib/package/language_extensions_packaging.py

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
None of the functions use instance state — they are pure functions.
77
"""
88

9-
import copy
109
import itertools
1110
import logging
1211
import re
@@ -24,7 +23,7 @@
2423
sanitize_resource_key_for_mapping,
2524
substitute_loop_variable,
2625
)
27-
from samcli.lib.cfn_language_extensions.utils import FOREACH_REQUIRED_ELEMENTS, is_foreach_key
26+
from samcli.lib.cfn_language_extensions.utils import FOREACH_REQUIRED_ELEMENTS, deep_thaw, is_foreach_key
2827

2928
LOG = logging.getLogger(__name__)
3029

@@ -66,7 +65,7 @@ def merge_language_extensions_s3_uris(
6665
dict
6766
The original template with updated S3 URIs
6867
"""
69-
result = copy.deepcopy(original_template)
68+
result = deep_thaw(original_template)
7069

7170
# Build a set of (foreach_key, property_name) tuples for dynamic properties
7271
dynamic_prop_keys: set = set()

samcli/lib/samlib/wrapper.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
from samcli.lib.cfn_language_extensions.models import (
2828
DynamicArtifactProperty,
2929
)
30+
from samcli.lib.cfn_language_extensions.utils import deep_thaw
3031
from samcli.lib.samlib.local_uri_plugin import SupportLocalUriPlugin
3132

3233
LOG = logging.getLogger(__name__)
@@ -122,7 +123,7 @@ def get_original_template(self) -> Dict[str, Any]:
122123
dict
123124
A deep copy of the original template with language extensions preserved
124125
"""
125-
return copy.deepcopy(self._original_template)
126+
return deep_thaw(self._original_template)
126127

127128
def get_dynamic_artifact_properties(self) -> List[DynamicArtifactProperty]:
128129
"""

tests/unit/lib/cfn_language_extensions/test_phase_separation.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -844,10 +844,11 @@ def test_result_preserves_original_template_structure(
844844
assert foreach_key in result.original_template["Resources"]
845845

846846
original_foreach = result.original_template["Resources"][foreach_key]
847-
assert isinstance(original_foreach, list)
847+
assert isinstance(original_foreach, (list, tuple))
848848
assert len(original_foreach) == 3
849849
assert original_foreach[0] == loop_var
850-
assert original_foreach[1] == collection
850+
# Collection may be a tuple (frozen) or list
851+
assert list(original_foreach[1]) == list(collection)
851852

852853
@pytest.mark.parametrize(
853854
"loop_name, loop_var, collection",

tests/unit/lib/cfn_language_extensions/test_sam_integration.py

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -624,16 +624,21 @@ def test_nonexistent_template_path_does_not_error(self):
624624
result = expand_language_extensions(template, template_path="/nonexistent/path/template.yaml")
625625
assert result.had_language_extensions is False
626626

627-
def test_non_language_extension_template_returns_independent_copies(self):
627+
def test_non_language_extension_template_returns_frozen_result(self):
628628
template = {"Resources": {}}
629629
result = expand_language_extensions(template)
630630
assert result.had_language_extensions is False
631-
# expanded_template should be an independent copy so mutations don't leak
632-
assert result.expanded_template is not result.original_template
633-
assert result.expanded_template == template
631+
# Both fields point to the same frozen object (no-LE path optimization)
632+
assert result.expanded_template is result.original_template
633+
assert dict(result.expanded_template) == template
634+
# Frozen — mutation raises TypeError
635+
with pytest.raises(TypeError):
636+
result.expanded_template["Resources"] = {}
634637

635638
def test_mutation_does_not_affect_subsequent_calls(self):
636-
"""Mutating a returned result must not affect subsequent calls."""
639+
"""Frozen results prevent mutation; callers must deep_thaw first."""
640+
from samcli.lib.cfn_language_extensions.utils import deep_thaw
641+
637642
template = {
638643
"Resources": {
639644
"MyStack": {
@@ -643,10 +648,11 @@ def test_mutation_does_not_affect_subsequent_calls(self):
643648
}
644649
}
645650
result1 = expand_language_extensions(template)
646-
# Simulate what update_template does: mutate Location in-place
647-
result1.expanded_template["Resources"]["MyStack"]["Properties"]["Location"] = "SomeOther/template.yaml"
651+
# Must deep_thaw before mutating (matches the documented contract)
652+
mutable = deep_thaw(result1.expanded_template)
653+
mutable["Resources"]["MyStack"]["Properties"]["Location"] = "SomeOther/template.yaml"
648654

649-
# A fresh template dict should not be affected
655+
# A fresh call should not be affected
650656
template2 = {
651657
"Resources": {
652658
"MyStack": {
@@ -922,7 +928,7 @@ def test_no_template_path_skips_cache(self):
922928
assert mock_process.call_count == 2
923929

924930
def test_cache_returns_independent_copies(self):
925-
"""Mutating a cached result must not affect subsequent cache hits."""
931+
"""Cached results are frozen — mutation raises TypeError."""
926932
with tempfile.TemporaryDirectory() as tmp:
927933
path = self._make_template_file(tmp)
928934
template = self._lang_ext_template()
@@ -939,11 +945,13 @@ def test_cache_returns_independent_copies(self):
939945
return_value=expanded,
940946
):
941947
result1 = expand_language_extensions(template, template_path=path)
942-
# Mutate the first result
943-
result1.expanded_template["Resources"]["TopicA"]["Properties"]["DisplayName"] = "mutated"
948+
# Frozen — mutation raises TypeError
949+
with pytest.raises(TypeError):
950+
result1.expanded_template["Resources"]["TopicA"]["Properties"]["DisplayName"] = "mutated"
944951

952+
# Cache hit returns the same frozen object
945953
result2 = expand_language_extensions(template, template_path=path)
946-
# Second result should have the original value
954+
assert result2 is result1
947955
assert result2.expanded_template["Resources"]["TopicA"]["Properties"]["DisplayName"] == "original"
948956

949957
def test_nonexistent_template_path_skips_cache(self):
@@ -972,8 +980,8 @@ def test_non_language_ext_template_cached(self):
972980
assert result1.had_language_extensions is False
973981
assert result2.had_language_extensions is False
974982
assert len(_expansion_cache) == 1
975-
# Ensure independence
976-
assert result1.expanded_template is not result2.expanded_template
983+
# Cache returns the same frozen object
984+
assert result1 is result2
977985

978986
def test_cache_evicts_oldest_when_full(self):
979987
"""Cache should evict the oldest entry when _MAX_CACHE_SIZE is reached."""

tests/unit/lib/samlib/test_wrapper.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,9 @@ def test_expand_language_extensions_success(self, mock_process):
143143
result = expand_language_extensions(input_template, parameter_values={"AWS::Region": "us-east-1"})
144144

145145
self.assertTrue(result.had_language_extensions)
146-
self.assertEqual(result.expanded_template, expected_output)
146+
from samcli.lib.cfn_language_extensions.utils import deep_thaw
147+
148+
self.assertEqual(deep_thaw(result.expanded_template), expected_output)
147149
mock_process.assert_called_once()
148150

149151
@patch("samcli.lib.cfn_language_extensions.sam_integration.process_template_for_sam_cli")

0 commit comments

Comments
 (0)