From 24555004e7ba92635988df5b2e7c16cc8f48b10f Mon Sep 17 00:00:00 2001 From: Jack McCarthy <37917934+Deicyde@users.noreply.github.com> Date: Thu, 15 Oct 2026 01:12:39 -0400 Subject: [PATCH 1/2] Guard #289's linear render and drop caches lazy links made redundant #289's scale test grew sections inside one chapter whose statements had no dependencies, so two of the call sites it was meant to guard could regress unnoticed. Copying a page's links in the chapter loop, or in _render_chapter's reads, kept the output identical and passed. At 4 against 8 sections the bound also missed quadratic terms below about three resolves per section squared. The test now gives every section a dependency and grows sections and, separately, chapters from 8 to 16. A new test fails if rendering iterates a page's links, and test_node_links_resolve_each_target_page_once checks that reading one node links only its own target page. Each of five reverts fails at least one of them: a dict() copy in the chapter loop, the scope loop, _write_page or _render_chapter's reads, and a mapping that builds every link on its first read. On pre-#289 code the scale test goes from 1,771 to 4,979 resolves. Laziness also made two caches redundant. _PageLinks memoized each link beside its per-target-page cache; it now builds a link on each read and answers `in` from the targets, so _view_links' membership test no longer builds one. _rewrite_links built a one-entry mapping per link, sharing a cache through _anchored_links' hrefs parameter; it now reads one mapping per call, which resolves its page only when a link is read. The output is byte-identical on the scale fixtures and the bundled example, and the 16-section fixture resolves 1,574 times instead of 1,607. --- autoform_cli/render.py | 43 +++++++-------- tests/test_graph_scale.py | 107 +++++++++++++++++++++++--------------- tests/test_render.py | 15 ++++-- 3 files changed, 95 insertions(+), 70 deletions(-) diff --git a/autoform_cli/render.py b/autoform_cli/render.py index b8578fb36..349a1814d 100644 --- a/autoform_cli/render.py +++ b/autoform_cli/render.py @@ -1943,45 +1943,43 @@ def _anchored_links( page: Path, *, extension: str = ".html", - hrefs: dict[Path, str] | None = None, ) -> Mapping[str, str]: """Link every node to its statement on the published chapter page. Use ``.md`` for links MkDocs will parse -- it validates and rewrites those itself -- and ``.html`` for raw HTML and Mermaid, which it never sees. A - statement on the current page is just a fragment. Calls for the same page - and extension can share *hrefs*, so each target page is linked once across - all of them. + statement on the current page is just a fragment. The mapping covers every + node but builds a link only when it is read, so read just the nodes the page + shows: iterating or copying it builds them all, and with a page per node + that makes rendering quadratic again. """ - return _PageLinks(targets, page, extension, {} if hrefs is None else hrefs) + return _PageLinks(targets, page, extension) class _PageLinks(Mapping[str, str]): - """The links from one page to every node, each built when first read. + """The links from one page to every node, each built when read. A page names a few nodes but *targets* holds the whole graph, and a site has a page per node, so building every link up front made rendering quadratic in the size of the graph. """ - def __init__(self, targets: dict[str, tuple[Path, str]], page: Path, extension: str, hrefs: dict[Path, str]): + def __init__(self, targets: dict[str, tuple[Path, str]], page: Path, extension: str) -> None: self._targets = targets self._page = page - self._resolved_page = page.resolve() + self._resolved_page: Path | None = None self._extension = extension - self._hrefs = hrefs - self._links: dict[str, str] = {} + self._hrefs: dict[Path, str] = {} def __getitem__(self, node_id: str) -> str: - link = self._links.get(node_id) - if link is not None: - return link target, anchor = self._targets[node_id] # Many nodes share a chapter page, and resolving a path walks the disk, # so each target page is linked once. The current page is cached as "" # and links as a bare fragment. href = self._hrefs.get(target) if href is None: + if self._resolved_page is None: + self._resolved_page = self._page.resolve() if target.resolve() == self._resolved_page: href = "" else: @@ -1990,11 +1988,11 @@ def __getitem__(self, node_id: str) -> str: href = _as_published(href) self._hrefs[target] = href if href: - link = f"{href}#{anchor}" if anchor else href - else: - link = f"#{anchor}" if anchor else "#" - self._links[node_id] = link - return link + return f"{href}#{anchor}" if anchor else href + return f"#{anchor}" if anchor else "#" + + def __contains__(self, node_id: object) -> bool: + return node_id in self._targets def __iter__(self) -> Iterator[str]: return iter(self._targets) @@ -2038,10 +2036,9 @@ def _rewrite_links( such as a source note's evidence link to a Lean file, keeps its text and loses its target instead of dangling. """ - # Most pages name a few nodes, so each node link is built where it is used. - # A coverage page can name most of the graph, so one cache serves the whole - # text and each target page it reaches is linked once. - page_hrefs: dict[Path, str] = {} + # A coverage page can name most of the graph, so one mapping serves the + # whole text and links each target page it reaches once. + links = _anchored_links(targets, page, extension=".md") def moved_target(raw: str) -> str | None: """Where *raw* should point once published, or None to leave it alone.""" @@ -2052,7 +2049,7 @@ def moved_target(raw: str) -> str | None: candidate = (source_dir / unquote(path)).resolve() node_id = node_sources.get(candidate) if node_id is not None: - href = _anchored_links({node_id: targets[node_id]}, page, extension=".md", hrefs=page_hrefs)[node_id] + href = links[node_id] if not targets[node_id][1] and separator: href = f"{'' if href == '#' else href}#{fragment}" return href diff --git a/tests/test_graph_scale.py b/tests/test_graph_scale.py index 4a92226b4..e20621a4b 100644 --- a/tests/test_graph_scale.py +++ b/tests/test_graph_scale.py @@ -546,16 +546,18 @@ def test_graph_page_publication_builds_whole_graph_indexes_a_fixed_number_of_tim assert max(small.values()) <= 4 -def _written_blueprint(tmp_path: Path, sections: int = 1) -> Path: +def _written_blueprint(tmp_path: Path, sections: int = 1, chapters: int = 1) -> Path: blueprint = tmp_path / "blueprint" - chapter = blueprint / "roadmap" / "chapter" + # One chapter keeps the "chapter" directory that other tests name. + groups = {"chapter": "Chapter"} if chapters == 1 else {f"chapter{n}": f"Chapter {n}" for n in range(chapters)} names = [f"section{index}" for index in range(sections)] - for name in names: - (chapter / name).mkdir(parents=True) + for group in groups: + for name in names: + (blueprint / "roadmap" / group / name).mkdir(parents=True) (blueprint / "README.md").write_text("# Book\n\n[Roadmap](roadmap/README.md)\n", encoding="utf-8") - (blueprint / "roadmap" / "README.md").write_text("# Roadmap\n\n[Chapter](chapter/README.md)\n", encoding="utf-8") - (chapter / "README.md").write_text( - "# Chapter\n\n" + "".join(f"[{name}]({name}/README.md)\n" for name in names), encoding="utf-8" + (blueprint / "roadmap" / "README.md").write_text( + "# Roadmap\n\n" + "".join(f"[{title}]({group}/README.md)\n" for group, title in groups.items()), + encoding="utf-8", ) (blueprint / "coverage").mkdir() (blueprint / "coverage" / "README.md").write_text( @@ -563,13 +565,18 @@ def _written_blueprint(tmp_path: Path, sections: int = 1) -> Path: "| Project scope | MAPPED | Source audit pending |\n", encoding="utf-8", ) - for name in names: - section = chapter / name - (section / "README.md").write_text(f"# {name}\n\n[A](a.md)\n[B](b.md)\n", encoding="utf-8") - (section / "a.md").write_text("---\ndeclaration: theorem\n---\n# A\n\nStatement.\n", encoding="utf-8") - (section / "b.md").write_text( - "---\ndeclaration: theorem\n---\n# B\n\nUses [A](a.md).\n", encoding="utf-8" + for group, title in groups.items(): + chapter = blueprint / "roadmap" / group + (chapter / "README.md").write_text( + f"# {title}\n\n" + "".join(f"[{name}]({name}/README.md)\n" for name in names), encoding="utf-8" ) + for name in names: + section = chapter / name + (section / "README.md").write_text(f"# {name}\n\n[A](a.md)\n[B](b.md)\n", encoding="utf-8") + (section / "a.md").write_text("---\ndeclaration: theorem\n---\n# A\n\nStatement.\n", encoding="utf-8") + (section / "b.md").write_text( + "---\ndeclaration: theorem\n---\n# B\n\nUses [A](a.md).\n", encoding="utf-8" + ) return blueprint @@ -592,57 +599,71 @@ def test_site_rendering_does_not_scan_for_children_per_node( assert (tmp_path / "site" / "dependencies" / "scopes" / "chapter" / "section0.md").is_file() -def _container_index_builds(tmp_path: Path, monkeypatch: pytest.MonkeyPatch, sections: int) -> int: - blueprint = _written_blueprint(tmp_path / str(sections), sections) - builds = 0 - original = render._containers +def _render_calls(blueprint: Path, monkeypatch: pytest.MonkeyPatch, owner: object, name: str) -> int: + """How often rendering *blueprint* calls ``owner.name``.""" + calls = 0 + original = getattr(owner, name) - def counted(graph: Graph) -> frozenset[str]: - nonlocal builds - builds += 1 - return original(graph) + def counted(*args, **kwargs): + nonlocal calls + calls += 1 + return original(*args, **kwargs) with monkeypatch.context() as patch: - patch.setattr(render, "_containers", counted) - render_site(blueprint, tmp_path / str(sections) / "site") - return builds + patch.setattr(owner, name, counted) + render_site(blueprint, blueprint.parent / "site") + return calls def test_site_rendering_indexes_containers_a_fixed_number_of_times( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, ) -> None: - assert _container_index_builds(tmp_path, monkeypatch, 6) == _container_index_builds(tmp_path, monkeypatch, 2) - + small, large = ( + _render_calls(_written_blueprint(tmp_path / str(sections), sections), monkeypatch, render, "_containers") + for sections in (2, 6) + ) -def _path_resolves(tmp_path: Path, monkeypatch: pytest.MonkeyPatch, sections: int) -> int: - blueprint = _written_blueprint(tmp_path / str(sections), sections) - calls = 0 - original = Path.resolve + assert large == small - def counted(self: Path, strict: bool = False) -> Path: - nonlocal calls - calls += 1 - return original(self, strict) - with monkeypatch.context() as patch: - patch.setattr(Path, "resolve", counted) - render_site(blueprint, tmp_path / str(sections) / "site") - return calls +def _path_resolves(tmp_path: Path, monkeypatch: pytest.MonkeyPatch, sections: int = 1, chapters: int = 1) -> int: + blueprint = _written_blueprint(tmp_path / f"{chapters}x{sections}", sections, chapters) + # With a dependency, each chapter page reads node links of its own, not only the maps. + for statement in blueprint.glob("roadmap/*/section*/b.md"): + with statement.open("a", encoding="utf-8") as text: + text.write("\n## Depends on\n\n- [A](a.md)\n") + return _render_calls(blueprint, monkeypatch, Path, "resolve") -def test_site_rendering_resolves_paths_linearly_in_the_number_of_sections( +@pytest.mark.parametrize("axis", ["sections", "chapters"]) +def test_site_rendering_resolves_paths_linearly_in_sections_and_chapters( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, + axis: str, ) -> None: - small = _path_resolves(tmp_path, monkeypatch, 4) - large = _path_resolves(tmp_path, monkeypatch, 8) + small = _path_resolves(tmp_path, monkeypatch, **{axis: 8}) + large = _path_resolves(tmp_path, monkeypatch, **{axis: 16}) - # Every statement has its own map page. Linking every node from every page - # made doubling the sections multiply the resolves by 2.4. + # Every statement and chapter has its own map page. Linking every node from + # every page made doubling the sections multiply the resolves by 2.8. assert large <= 2 * small +def test_site_rendering_never_iterates_a_pages_node_links( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + # A page's node links cover the whole graph and are built when read, so + # iterating or copying them on every page makes render quadratic again. + def iterated(self: object) -> None: + raise AssertionError("a page's node links were iterated") + + monkeypatch.setattr(render._PageLinks, "__iter__", iterated) + + render_site(_written_blueprint(tmp_path, 2, chapters=2), tmp_path / "site") + + def test_audit_does_not_scan_for_children_per_node(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: graph = load_graph(_written_blueprint(tmp_path)) expected = audit_graph(graph) diff --git a/tests/test_render.py b/tests/test_render.py index 21f1ee2ec..d3887841c 100644 --- a/tests/test_render.py +++ b/tests/test_render.py @@ -1445,12 +1445,18 @@ def counting_resolve(self: Path, strict: bool = False) -> Path: links = _anchored_links(targets, page) - # A page reads only the nodes it shows, so nothing is linked before then. + # A page reads only the nodes it shows, so nothing is linked before then, + # and reading one node links only its own target page. assert calls == [] assert links["a/7"] == "a.html#n7" + assert calls == [chapter] assert links["b"] == "b/index.html" assert links["here"] == "#self" - assert len(dict(links)) == len(targets) + assert dict(links) == { + **{f"a/{index}": f"a.html#n{index}" for index in range(20)}, + "b": "b/index.html", + "here": "#self", + } assert sorted(calls) == [chapter, other] # Once to compare it with the current page, once inside relative_link. assert resolved.count(chapter) <= 2 @@ -1495,8 +1501,9 @@ def rewrite(text: str) -> str: def test_a_focus_page_asks_for_its_node_links_once(tmp_path: Path) -> None: - """Each request builds a link for every node, so asking twice per focus - page doubled the cost of the largest group of generated pages.""" + """Each request resolves the page, and every target page it reads, afresh, + so asking twice per focus page repeated that work on the largest group of + generated pages.""" from autoform_cli.graph_pages import focus_page_path, write_graph_pages project = _project(tmp_path) From cd7e78b441d7602bda86bea2977c5bfd9c7bc4d0 Mon Sep 17 00:00:00 2001 From: Jack McCarthy Date: Sat, 17 Oct 2026 13:57:03 -0400 Subject: [PATCH 2/2] Apply the review: guard the chapter page's link reads - The never-iterate guard now renders statements with a dependency, so `_render_chapter`'s node links are read inside it. A `dict(links)` in `_node_references` passed the guard before and fails it now. `_written_blueprint` writes the dependency itself, and `_path_resolves` no longer appends it. - Pin the lazy page resolve: building the mapping resolves nothing. - `_anchored_links`' docstring says iterating walks the graph, the link tail is one expression, and the scale comment covers chapters too. --- autoform_cli/render.py | 11 ++++------- tests/test_graph_scale.py | 20 ++++++++++---------- tests/test_render.py | 2 +- 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/autoform_cli/render.py b/autoform_cli/render.py index 349a1814d..2bcfe5aba 100644 --- a/autoform_cli/render.py +++ b/autoform_cli/render.py @@ -1948,10 +1948,9 @@ def _anchored_links( Use ``.md`` for links MkDocs will parse -- it validates and rewrites those itself -- and ``.html`` for raw HTML and Mermaid, which it never sees. A - statement on the current page is just a fragment. The mapping covers every - node but builds a link only when it is read, so read just the nodes the page - shows: iterating or copying it builds them all, and with a page per node - that makes rendering quadratic again. + statement on the current page is just a fragment. Read only the nodes the + page shows: links are built when read, and iterating or copying the mapping + walks the whole graph on every page. """ return _PageLinks(targets, page, extension) @@ -1987,9 +1986,7 @@ def __getitem__(self, node_id: str) -> str: if self._extension == ".html": href = _as_published(href) self._hrefs[target] = href - if href: - return f"{href}#{anchor}" if anchor else href - return f"#{anchor}" if anchor else "#" + return f"{href}#{anchor}" if anchor else href or "#" def __contains__(self, node_id: object) -> bool: return node_id in self._targets diff --git a/tests/test_graph_scale.py b/tests/test_graph_scale.py index e20621a4b..f216b2add 100644 --- a/tests/test_graph_scale.py +++ b/tests/test_graph_scale.py @@ -546,7 +546,7 @@ def test_graph_page_publication_builds_whole_graph_indexes_a_fixed_number_of_tim assert max(small.values()) <= 4 -def _written_blueprint(tmp_path: Path, sections: int = 1, chapters: int = 1) -> Path: +def _written_blueprint(tmp_path: Path, sections: int = 1, chapters: int = 1, dependencies: bool = False) -> Path: blueprint = tmp_path / "blueprint" # One chapter keeps the "chapter" directory that other tests name. groups = {"chapter": "Chapter"} if chapters == 1 else {f"chapter{n}": f"Chapter {n}" for n in range(chapters)} @@ -574,8 +574,11 @@ def _written_blueprint(tmp_path: Path, sections: int = 1, chapters: int = 1) -> section = chapter / name (section / "README.md").write_text(f"# {name}\n\n[A](a.md)\n[B](b.md)\n", encoding="utf-8") (section / "a.md").write_text("---\ndeclaration: theorem\n---\n# A\n\nStatement.\n", encoding="utf-8") + # With a dependency, each chapter page reads node links of its own, not only the maps. (section / "b.md").write_text( - "---\ndeclaration: theorem\n---\n# B\n\nUses [A](a.md).\n", encoding="utf-8" + "---\ndeclaration: theorem\n---\n# B\n\nUses [A](a.md).\n" + + ("\n## Depends on\n\n- [A](a.md)\n" if dependencies else ""), + encoding="utf-8", ) return blueprint @@ -628,11 +631,7 @@ def test_site_rendering_indexes_containers_a_fixed_number_of_times( def _path_resolves(tmp_path: Path, monkeypatch: pytest.MonkeyPatch, sections: int = 1, chapters: int = 1) -> int: - blueprint = _written_blueprint(tmp_path / f"{chapters}x{sections}", sections, chapters) - # With a dependency, each chapter page reads node links of its own, not only the maps. - for statement in blueprint.glob("roadmap/*/section*/b.md"): - with statement.open("a", encoding="utf-8") as text: - text.write("\n## Depends on\n\n- [A](a.md)\n") + blueprint = _written_blueprint(tmp_path / f"{chapters}x{sections}", sections, chapters, dependencies=True) return _render_calls(blueprint, monkeypatch, Path, "resolve") @@ -645,8 +644,9 @@ def test_site_rendering_resolves_paths_linearly_in_sections_and_chapters( small = _path_resolves(tmp_path, monkeypatch, **{axis: 8}) large = _path_resolves(tmp_path, monkeypatch, **{axis: 16}) - # Every statement and chapter has its own map page. Linking every node from - # every page made doubling the sections multiply the resolves by 2.8. + # Every statement, section and chapter has its own map page. Linking every + # node from every page made doubling the sections or chapters multiply the + # resolves by 2.8 or 3.2. assert large <= 2 * small @@ -661,7 +661,7 @@ def iterated(self: object) -> None: monkeypatch.setattr(render._PageLinks, "__iter__", iterated) - render_site(_written_blueprint(tmp_path, 2, chapters=2), tmp_path / "site") + render_site(_written_blueprint(tmp_path, 2, chapters=2, dependencies=True), tmp_path / "site") def test_audit_does_not_scan_for_children_per_node(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: diff --git a/tests/test_render.py b/tests/test_render.py index d3887841c..388ef33aa 100644 --- a/tests/test_render.py +++ b/tests/test_render.py @@ -1447,7 +1447,7 @@ def counting_resolve(self: Path, strict: bool = False) -> Path: # A page reads only the nodes it shows, so nothing is linked before then, # and reading one node links only its own target page. - assert calls == [] + assert calls == resolved == [] assert links["a/7"] == "a.html#n7" assert calls == [chapter] assert links["b"] == "b/index.html"