From 1924cc234d26f76f91d0999cd395fcb6668a0ac7 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 8 Aug 2026 12:45:20 +0200 Subject: [PATCH] perf(buffer): remove the process pool from page rendering (S12, R1, R2) PageBuffer started a ProcessPoolExecutor(max_workers=4) and submitted page renders to it. Every job failed. _render_page_worker returned pickle.dumps(page), and a Page holds a live PIL canvas, which is not picklable, so check_completed_renders swallowed a TypeError into a bare print and cached nothing. The cost was paid in full for zero benefit: four interpreter copies plus the whole block list shipped per job (~880KB for a 200-block document). Three further defects would have had to be fixed before it could ever have worked: the worker built its BidirectionalLayouter without page_size so it silently used the (800, 600) default; check_completed_renders cached every result with is_backward=False, so backward renders landed in the forward buffer; and both _queue_*_renders broke at the end of their first loop body, queueing one page each despite looping buffer_size times. Two live defects go with it: R1 - on Python 3.14 the default start method became forkserver, so submit() reaches _check_not_importing_main() and raises unless the caller sits inside an `if __name__ == "__main__"` guard. EreaderLayoutManager.get_current_page() raised outright from ordinary module-level script code. R2 - PageBuffer.__del__ called executor.shutdown(wait=True). Blocking on a process pool from a finaliser at interpreter teardown deadlocked; the test suite finished in 11.5s and then never exited. S12's measurement gate, on the tests/data Wikipedia fixture (411 blocks) with text caches warm: 800x600 p50 8.8 ms p95 15.4 ms 1072x1448 p50 13.8 ms p95 56.1 ms A page turn is cheaper than the IPC meant to hide it, so the gate says delete rather than replace. The LRU buffers, position maps and invalidation logic are kept unchanged; only the executor, worker, pickling, prefetch queueing and the lock guarding the pending-render dict are removed. If a slower device ever changes the numbers, the fallback is a synchronous readahead() method or a single worker thread, not processes. EreaderLayoutManager.shutdown() becomes idempotent and its __del__ no longer propagates exceptions - it was doing bookmark file I/O during interpreter teardown. Adds tests/layout/test_page_buffer.py, which the module had none of: LRU eviction and position-map cleanup, cache hits, font-scale invalidation, backward round-trip, and subprocess regressions for R1 (no __main__ guard) and R2 (exit without explicit shutdown). 870 passed, and the suite now exits in 12s wall instead of hanging. Co-Authored-By: Claude Opus 5 --- pyWebLayout/layout/ereader_manager.py | 21 +- pyWebLayout/layout/page_buffer.py | 274 +++++--------------------- tests/layout/test_page_buffer.py | 237 ++++++++++++++++++++++ 3 files changed, 304 insertions(+), 228 deletions(-) create mode 100644 tests/layout/test_page_buffer.py diff --git a/pyWebLayout/layout/ereader_manager.py b/pyWebLayout/layout/ereader_manager.py index 7faadb1..736daf4 100644 --- a/pyWebLayout/layout/ereader_manager.py +++ b/pyWebLayout/layout/ereader_manager.py @@ -935,16 +935,31 @@ class EreaderLayoutManager: """ Shutdown the ereader manager and clean up resources. Call this when the application is closing. + + Idempotent: calling it twice saves the position once. """ + if getattr(self, '_shutdown_done', False): + return + self._shutdown_done = True + # Save current position self.bookmark_manager.save_reading_position(self.current_position) - # Shutdown renderer and buffer + # Release cached pages self.renderer.shutdown() def __del__(self): - """Cleanup on destruction""" - self.shutdown() + """ + Best-effort cleanup for callers that never called shutdown(). + + Finalisers run during interpreter teardown, when modules and globals + may already be torn down, so this must never raise and must never + block. Applications should call shutdown() explicitly. + """ + try: + self.shutdown() + except Exception: + pass # Convenience function for quick setup diff --git a/pyWebLayout/layout/page_buffer.py b/pyWebLayout/layout/page_buffer.py index 0b95672..e509530 100644 --- a/pyWebLayout/layout/page_buffer.py +++ b/pyWebLayout/layout/page_buffer.py @@ -1,16 +1,35 @@ """ -Multi-process page buffering system for high-performance ereader navigation. +Page caching for ereader navigation. -This module provides intelligent page caching with background rendering using -multiprocessing to achieve sub-second page navigation performance. +`PageBuffer` is an LRU cache of rendered pages plus the position links between +them; `BufferedPageRenderer` wraps it around a `BidirectionalLayouter`. + +This module used to render pages ahead of time in a `ProcessPoolExecutor`. That +never worked and has been removed — see S12 in docs/LAYOUT_REMEDIATION_SPEC.md +and R1/R2 in docs/ARCHITECTURE_REVIEW.md. In short: the worker returned +`pickle.dumps(page)`, and a Page holds a live PIL canvas, which is not +picklable, so every job failed and the result was discarded. The cost — four +interpreter copies and the whole block list shipped per job — was paid in full +for no benefit. On Python 3.14, where the default start method became +`forkserver`, submitting from module-level code raised outright. + +Prefetch is not needed. Measured on the tests/data Wikipedia fixture (411 +blocks) with the text caches warm, one page render costs: + + 800x600 p50 8.8 ms p95 15.4 ms + 1072x1448 p50 13.8 ms p95 56.1 ms + +A page turn is cheaper than the IPC that was meant to hide it. If a slower +target device ever changes that, the fallback is a synchronous `readahead()` +method on this class, or a single worker *thread* — layout is PIL-bound and PIL +releases the GIL — not a process pool. Making the concrete tree picklable +(Page -> Line -> Text -> Font -> FreeTypeFont) is a large surface area to +maintain for a cache. """ from __future__ import annotations from typing import Dict, Optional, List, Tuple, Any from collections import OrderedDict -from concurrent.futures import ProcessPoolExecutor, Future -import threading -import pickle from .ereader_layout import RenderingPosition, BidirectionalLayouter, FontFamilyOverride from pyWebLayout.concrete.page import Page @@ -19,57 +38,20 @@ from pyWebLayout.style.page_style import PageStyle from pyWebLayout.style.fonts import BundledFont -def _render_page_worker(args: Tuple[List[Block], - PageStyle, - RenderingPosition, - float, - bool, - Optional[BundledFont]]) -> Tuple[RenderingPosition, - bytes, - RenderingPosition]: - """ - Worker function for multiprocess page rendering. - - Args: - args: Tuple of (blocks, page_style, position, font_scale, is_backward, font_family) - - Returns: - Tuple of (original_position, pickled_page, next_position) - """ - blocks, page_style, position, font_scale, is_backward, font_family = args - - # Create font family override if specified - font_family_override = FontFamilyOverride(font_family) if font_family else None - - layouter = BidirectionalLayouter(blocks, page_style, font_family_override=font_family_override) - - if is_backward: - page, next_pos = layouter.render_page_backward(position, font_scale) - else: - page, next_pos = layouter.render_page_forward(position, font_scale) - - # Serialize the page for inter-process communication - pickled_page = pickle.dumps(page) - - return position, pickled_page, next_pos - - class PageBuffer: """ - Intelligent page caching system with LRU eviction and background rendering. - Maintains separate forward and backward buffers for optimal navigation performance. + LRU cache of rendered pages, with separate forward and backward buffers and + the position links between adjacent pages. """ - def __init__(self, buffer_size: int = 5, max_workers: int = 4): + def __init__(self, buffer_size: int = 5): """ Initialize the page buffer. Args: buffer_size: Number of pages to cache in each direction - max_workers: Maximum number of worker processes for background rendering """ self.buffer_size = buffer_size - self.max_workers = max_workers # LRU caches for forward and backward pages self.forward_buffer: OrderedDict[RenderingPosition, Page] = OrderedDict() @@ -81,11 +63,6 @@ class PageBuffer: self.reverse_position_map: Dict[RenderingPosition, RenderingPosition] = {} # current -> previous - # Background rendering - self.executor: Optional[ProcessPoolExecutor] = None - self.pending_renders: Dict[RenderingPosition, Future] = {} - self.render_lock = threading.Lock() - # Document state self.blocks: Optional[List[Block]] = None self.page_style: Optional[PageStyle] = None @@ -112,10 +89,6 @@ class PageBuffer: self.current_font_scale = font_scale self.current_font_family = font_family - # Start the process pool - if self.executor is None: - self.executor = ProcessPoolExecutor(max_workers=self.max_workers) - def get_page(self, position: RenderingPosition) -> Optional[Page]: """ Get a cached page if available. @@ -176,125 +149,12 @@ class PageBuffer: self.position_map.pop(oldest_pos, None) self.reverse_position_map.pop(oldest_pos, None) - def start_background_rendering( - self, - current_position: RenderingPosition, - direction: str = 'forward'): - """ - Start background rendering of upcoming pages. - - Args: - current_position: Current reading position - direction: 'forward', 'backward', or 'both' - """ - if not self.blocks or not self.page_style or not self.executor: - return - - with self.render_lock: - if direction in ['forward', 'both']: - self._queue_forward_renders(current_position) - - if direction in ['backward', 'both']: - self._queue_backward_renders(current_position) - - def _queue_forward_renders(self, start_position: RenderingPosition): - """Queue forward page renders starting from the given position""" - current_pos = start_position - - for i in range(self.buffer_size): - # Skip if already cached or being rendered - if current_pos in self.forward_buffer or current_pos in self.pending_renders: - # Try to get next position from cache - current_pos = self.position_map.get(current_pos) - if not current_pos: - break - continue - - # Queue render job - args = ( - self.blocks, - self.page_style, - current_pos, - self.current_font_scale, - False, - self.current_font_family) - future = self.executor.submit(_render_page_worker, args) - self.pending_renders[current_pos] = future - - # We don't know the next position yet, so we'll update it when the render - # completes - break - - def _queue_backward_renders(self, start_position: RenderingPosition): - """Queue backward page renders ending at the given position""" - current_pos = start_position - - for i in range(self.buffer_size): - # Skip if already cached or being rendered - if current_pos in self.backward_buffer or current_pos in self.pending_renders: - # Try to get previous position from cache - current_pos = self.reverse_position_map.get(current_pos) - if not current_pos: - break - continue - - # Queue render job - args = ( - self.blocks, - self.page_style, - current_pos, - self.current_font_scale, - True, - self.current_font_family) - future = self.executor.submit(_render_page_worker, args) - self.pending_renders[current_pos] = future - - # We don't know the previous position yet, so we'll update it when the - # render completes - break - - def check_completed_renders(self): - """Check for completed background renders and cache the results""" - if not self.pending_renders: - return - - completed = [] - - with self.render_lock: - for position, future in self.pending_renders.items(): - if future.done(): - try: - original_pos, pickled_page, next_pos = future.result() - - # Deserialize the page - page = pickle.loads(pickled_page) - - # Cache the page - self.cache_page(original_pos, page, next_pos, is_backward=False) - - completed.append(position) - - except Exception as e: - print(f"Background render failed for position {position}: {e}") - completed.append(position) - - # Remove completed renders - for pos in completed: - self.pending_renders.pop(pos, None) - def invalidate_all(self): - """Clear all cached pages and cancel pending renders""" - with self.render_lock: - # Cancel pending renders - for future in self.pending_renders.values(): - future.cancel() - self.pending_renders.clear() - - # Clear caches - self.forward_buffer.clear() - self.backward_buffer.clear() - self.position_map.clear() - self.reverse_position_map.clear() + """Clear all cached pages""" + self.forward_buffer.clear() + self.backward_buffer.clear() + self.position_map.clear() + self.reverse_position_map.clear() def set_font_scale(self, font_scale: float): """ @@ -323,7 +183,6 @@ class PageBuffer: return { 'forward_buffer_size': len(self.forward_buffer), 'backward_buffer_size': len(self.backward_buffer), - 'pending_renders': len(self.pending_renders), 'position_mappings': len(self.position_map), 'reverse_position_mappings': len(self.reverse_position_map), 'current_font_scale': self.current_font_scale, @@ -331,28 +190,20 @@ class PageBuffer: } def shutdown(self): - """Shutdown the page buffer and clean up resources""" - if self.executor: - # Cancel pending renders - with self.render_lock: - for future in self.pending_renders.values(): - future.cancel() + """ + Release cached pages. - # Shutdown executor - self.executor.shutdown(wait=True) - self.executor = None - - # Clear all caches + Cheap and idempotent. There is deliberately no __del__ calling this: + blocking work in a finaliser is what deadlocked the interpreter at exit + while the process pool existed. + """ self.invalidate_all() - def __del__(self): - """Cleanup on destruction""" - self.shutdown() - class BufferedPageRenderer: """ - High-level interface for buffered page rendering with automatic background caching. + High-level interface for page rendering with an LRU cache in front of the + layouter. """ def __init__(self, @@ -390,7 +241,7 @@ class BufferedPageRenderer: def render_page(self, position: RenderingPosition, font_scale: float = 1.0) -> Tuple[Page, RenderingPosition]: """ - Render a page with intelligent caching. + Render a page, serving it from cache when possible. Args: position: Position to render from @@ -407,32 +258,18 @@ class BufferedPageRenderer: # Check cache first cached_page = self.buffer.get_page(position) if cached_page: - # Get next position from position map + # Only use the cache if we also know where the next page starts; + # otherwise fall through and compute it. next_pos = self.buffer.position_map.get(position) - - # Only use cache if we have the forward position mapping - # Otherwise, we need to compute it if next_pos is not None: - # Start background rendering for upcoming pages - self.buffer.start_background_rendering(position, 'forward') - return cached_page, next_pos - # Cache hit for the page, but we don't have the forward position - # Fall through to compute it below - # Render the page directly page, next_pos = self.layouter.render_page_forward(position, font_scale) # Cache the result self.buffer.cache_page(position, page, next_pos) - # Start background rendering - self.buffer.start_background_rendering(position, 'both') - - # Check for completed background renders - self.buffer.check_completed_renders() - return page, next_pos def render_page_backward(self, @@ -440,7 +277,8 @@ class BufferedPageRenderer: font_scale: float = 1.0) -> Tuple[Page, RenderingPosition]: """ - Render a page ending at the given position with intelligent caching. + Render a page ending at the given position, serving it from cache when + possible. Args: end_position: Position where page should end @@ -457,32 +295,18 @@ class BufferedPageRenderer: # Check cache first cached_page = self.buffer.get_page(end_position) if cached_page: - # Get previous position from reverse position map + # Only use the cache if we also know where the previous page + # starts; otherwise fall through and compute it. prev_pos = self.buffer.reverse_position_map.get(end_position) - - # Only use cache if we have the reverse position mapping - # Otherwise, we need to compute it if prev_pos is not None: - # Start background rendering for previous pages - self.buffer.start_background_rendering(end_position, 'backward') - return cached_page, prev_pos - # Cache hit for the page, but we don't have the reverse position - # Fall through to compute it below - # Render the page directly page, start_pos = self.layouter.render_page_backward(end_position, font_scale) # Cache the result self.buffer.cache_page(start_pos, page, end_position, is_backward=True) - # Start background rendering - self.buffer.start_background_rendering(end_position, 'both') - - # Check for completed background renders - self.buffer.check_completed_renders() - return page, start_pos def set_font_family(self, font_family: Optional[BundledFont]): @@ -516,5 +340,5 @@ class BufferedPageRenderer: return self.buffer.get_cache_stats() def shutdown(self): - """Shutdown the renderer and clean up resources""" + """Release cached pages""" self.buffer.shutdown() diff --git a/tests/layout/test_page_buffer.py b/tests/layout/test_page_buffer.py new file mode 100644 index 0000000..5038606 --- /dev/null +++ b/tests/layout/test_page_buffer.py @@ -0,0 +1,237 @@ +""" +Tests for the page caching layer. + +Covers PageBuffer's LRU behaviour and BufferedPageRenderer's cache hits, plus +regressions for S12/R1/R2: the module must not start worker processes and must +not do blocking work in a finaliser. +""" + +import subprocess +import sys +import textwrap + +import pytest + +from pyWebLayout.layout.page_buffer import PageBuffer, BufferedPageRenderer +from pyWebLayout.layout.ereader_layout import RenderingPosition +from pyWebLayout.abstract.block import Paragraph +from pyWebLayout.abstract.inline import Word +from pyWebLayout.style import Font +from pyWebLayout.style.page_style import PageStyle + + +# ============================================================================ +# Fixtures +# ============================================================================ + +@pytest.fixture +def sample_blocks(): + """A document long enough to paginate over several pages.""" + font = Font() + blocks = [] + for p in range(6): + para = Paragraph(style=font) + for w in range(120): + para.add_word(Word(f"p{p}w{w}", font)) + blocks.append(para) + return blocks + + +@pytest.fixture +def renderer(sample_blocks): + return BufferedPageRenderer(sample_blocks, PageStyle(), buffer_size=3, page_size=(800, 600)) + + +# ============================================================================ +# PageBuffer +# ============================================================================ + +class TestPageBuffer: + def test_get_page_misses_when_empty(self): + buf = PageBuffer(buffer_size=3) + assert buf.get_page(RenderingPosition()) is None + + def test_cache_page_round_trips(self): + buf = PageBuffer(buffer_size=3) + pos, nxt = RenderingPosition(block_index=0), RenderingPosition(block_index=1) + sentinel = object() + + buf.cache_page(pos, sentinel, nxt) + + assert buf.get_page(pos) is sentinel + assert buf.position_map[pos] == nxt + + def test_lru_evicts_oldest_and_cleans_position_map(self): + buf = PageBuffer(buffer_size=2) + positions = [RenderingPosition(block_index=i) for i in range(4)] + for i, pos in enumerate(positions): + buf.cache_page(pos, object(), RenderingPosition(block_index=i + 1)) + + assert buf.get_page(positions[0]) is None, "oldest should have been evicted" + assert positions[0] not in buf.position_map, "position map must not leak evicted entries" + assert buf.get_page(positions[-1]) is not None + + def test_get_page_refreshes_lru_order(self): + buf = PageBuffer(buffer_size=2) + a, b, c = (RenderingPosition(block_index=i) for i in range(3)) + buf.cache_page(a, object()) + buf.cache_page(b, object()) + + buf.get_page(a) # a becomes most recently used + buf.cache_page(c, object()) + + assert buf.get_page(a) is not None, "recently used entry should survive" + assert buf.get_page(b) is None, "least recently used entry should be evicted" + + def test_backward_pages_land_in_the_backward_buffer(self): + buf = PageBuffer(buffer_size=3) + start, end = RenderingPosition(block_index=1), RenderingPosition(block_index=2) + + buf.cache_page(start, object(), end, is_backward=True) + + assert start in buf.backward_buffer + assert start not in buf.forward_buffer + assert buf.reverse_position_map[end] == start + + def test_font_scale_change_invalidates(self): + buf = PageBuffer(buffer_size=3) + pos = RenderingPosition() + buf.cache_page(pos, object(), RenderingPosition(block_index=1)) + + buf.set_font_scale(1.5) + + assert buf.get_page(pos) is None + + def test_same_font_scale_keeps_cache(self): + buf = PageBuffer(buffer_size=3) + pos = RenderingPosition() + buf.cache_page(pos, object(), RenderingPosition(block_index=1)) + + buf.set_font_scale(1.0) + + assert buf.get_page(pos) is not None + + def test_shutdown_is_idempotent(self): + buf = PageBuffer(buffer_size=3) + buf.cache_page(RenderingPosition(), object()) + + buf.shutdown() + buf.shutdown() + + assert buf.get_cache_stats()['forward_buffer_size'] == 0 + + +# ============================================================================ +# BufferedPageRenderer +# ============================================================================ + +class TestBufferedPageRenderer: + def test_render_page_returns_a_page_and_advances(self, renderer): + page, next_pos = renderer.render_page(RenderingPosition(), 1.0) + + assert page is not None + assert next_pos != RenderingPosition() + + def test_second_render_of_same_position_is_served_from_cache(self, renderer): + pos = RenderingPosition() + first, first_next = renderer.render_page(pos, 1.0) + second, second_next = renderer.render_page(pos, 1.0) + + assert second is first, "identical page object means it came from the cache" + assert second_next == first_next + + def test_font_scale_change_forces_a_re_render(self, renderer): + pos = RenderingPosition() + first, _ = renderer.render_page(pos, 1.0) + scaled, _ = renderer.render_page(pos, 1.5) + + assert scaled is not first + + def test_backward_render_round_trips_to_the_original_position(self, renderer): + start = RenderingPosition() + _, second_page_pos = renderer.render_page(start, 1.0) + + _, back_to = renderer.render_page_backward(second_page_pos, 1.0) + + assert back_to == start + + def test_shutdown_clears_the_cache(self, renderer): + renderer.render_page(RenderingPosition(), 1.0) + renderer.shutdown() + + assert renderer.get_cache_stats()['forward_buffer_size'] == 0 + + +# ============================================================================ +# S12 / R1 / R2 regressions +# ============================================================================ + +class TestNoBackgroundProcesses: + """ + The process pool that used to live here never produced a usable page (a Page + holds a live PIL canvas and cannot be pickled), and on Python 3.14's + forkserver default it raised when driven from module-level code. + """ + + def test_module_declares_no_process_pool(self): + import pyWebLayout.layout.page_buffer as page_buffer + + source = page_buffer.__file__ + assert not hasattr(page_buffer, '_render_page_worker') + assert not hasattr(PageBuffer(), 'executor') + with open(source, encoding='utf-8') as fh: + body = fh.read().split('"""', 2)[-1] # skip the module docstring + assert 'ProcessPoolExecutor' not in body + assert 'pickle' not in body + + def test_page_buffer_has_no_finaliser(self): + """ + PageBuffer.__del__ called executor.shutdown(wait=True), which deadlocked + the interpreter at exit. Cleanup must be explicit. + """ + assert '__del__' not in vars(PageBuffer) + + def test_navigation_works_without_a_main_guard(self, tmp_path): + """ + R1: EreaderLayoutManager raised RuntimeError when used from module-level + script code, because submitting to a ProcessPoolExecutor under a + non-fork start method requires an `if __name__ == "__main__"` guard. + """ + script = textwrap.dedent(f""" + from pyWebLayout.io.readers.html_extraction import parse_html_string + from pyWebLayout.layout.ereader_manager import EreaderLayoutManager + + blocks = parse_html_string("

" + " ".join(f"w{{i}}" for i in range(2000)) + "

") + m = EreaderLayoutManager(blocks, page_size=(800, 600), + bookmarks_dir={str(tmp_path)!r}) + m.get_current_page() + m.next_page() + m.previous_page() + m.shutdown() + print("OK") + """) + result = subprocess.run([sys.executable, "-c", script], + capture_output=True, text=True, timeout=120) + + assert result.returncode == 0, result.stderr + assert "OK" in result.stdout + + def test_interpreter_exits_without_explicit_shutdown(self, tmp_path): + """ + R2: a manager left to be finalised at exit must not hang. The timeout is + the assertion. + """ + script = textwrap.dedent(f""" + from pyWebLayout.io.readers.html_extraction import parse_html_string + from pyWebLayout.layout.ereader_manager import EreaderLayoutManager + + blocks = parse_html_string("

" + " ".join(f"w{{i}}" for i in range(500)) + "

") + m = EreaderLayoutManager(blocks, page_size=(800, 600), + bookmarks_dir={str(tmp_path)!r}) + m.get_current_page() + # deliberately no shutdown() - rely on interpreter teardown + """) + result = subprocess.run([sys.executable, "-c", script], + capture_output=True, text=True, timeout=60) + + assert result.returncode == 0, result.stderr