dtourolle/pyWebLayout
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

perf(buffer): remove the process pool from page rendering (S12, R1, R2)
Python CI / test (3.10) (push) Has been cancelled
Details
Python CI / test (3.12) (push) Has been cancelled
Details
Python CI / test (3.13) (push) Has been cancelled
Details

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-08 12:45:20 +02:00
co-authored by Claude Opus 5
parent 456824d6d6
commit 1924cc234d
3 changed files with 304 additions and 228 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pyWebLayout/layout/ereader_manager.py
+17 -2
View File
@@ -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"""
"""
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
pyWebLayout/layout/page_buffer.py
+45 -221
View File
@@ -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,121 +149,8 @@ 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
"""Clear all cached pages"""
self.forward_buffer.clear()
self.backward_buffer.clear()
self.position_map.clear()
@@ -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()
tests/layout/test_page_buffer.py
+237
View File
@@ -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("<p>" + " ".join(f"w{{i}}" for i in range(2000)) + "</p>")
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("<p>" + " ".join(f"w{{i}}" for i in range(500)) + "</p>")
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