Skip to content

perf: Add caching layer for rendering operations #1287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
CybotTM wants to merge 1 commit into
base: main
Choose a base branch
from
Open

perf: Add caching layer for rendering operations #1287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions packages/guides/src/NodeRenderers/PreRenderers/PreNodeRendererFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,16 @@

/**
* Decorator to add pre-rendering logic to node renderers.
*
* Note: Caching assumes PreNodeRenderer::supports() only checks the node's
* class type, not instance-specific properties. If a PreNodeRenderer needs
* to check node properties, caching by class would return incorrect results.
Comment on lines +24 to +27
Copy link
Member

@jaapio jaapio Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot make sure nobody is relying on this behavior. I concider this to be a breaking change. I see how this could improve the speed, but the actual impact of this change might be limited.

What we could do is introduce a new interface that can be implemented by the PreNodeRenderer that already exist.
PreNodeRendererCachableSupports would have a method cacheSupport() : bool

When preRenderers is not empty and all preRenderers are cachable, we can create add it to cache.

I think that will make sure we are backward compatible but still can cache the node renderers.

*/
final class PreNodeRendererFactory implements NodeRendererFactory
{
/** @var array<class-string<Node>, NodeRenderer<Node>> */
private array $cache = [];

public function __construct(
private readonly NodeRendererFactory $innerFactory,
/** @var iterable<PreNodeRenderer> */
Expand All @@ -33,6 +40,12 @@ public function __construct(

public function get(Node $node): NodeRenderer
{
// Cache by node class to avoid repeated preRenderer iteration
$nodeFqcn = $node::class;
if (isset($this->cache[$nodeFqcn])) {
return $this->cache[$nodeFqcn];
}

$preRenderers = [];
foreach ($this->preRenderers as $preRenderer) {
if (!$preRenderer->supports($node)) {
Expand All @@ -43,9 +56,9 @@ public function get(Node $node): NodeRenderer
}

if (count($preRenderers) === 0) {
return $this->innerFactory->get($node);
return $this->cache[$nodeFqcn] = $this->innerFactory->get($node);
}

return new PreRenderer($this->innerFactory->get($node), $preRenderers);
return $this->cache[$nodeFqcn] = new PreRenderer($this->innerFactory->get($node), $preRenderers);
}
}
5 changes: 5 additions & 0 deletions packages/guides/src/Nodes/DocumentNode.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,11 @@ public function getFootnoteTargetAnonymous(): FootnoteTarget|null
return null;
}

public function hasDocumentEntry(): bool
{
return $this->documentEntry !== null;
}

public function getDocumentEntry(): DocumentEntryNode
{
if ($this->documentEntry === null) {
Expand Down
46 changes: 37 additions & 9 deletions packages/guides/src/ReferenceResolvers/DocumentNameResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,18 @@

final class DocumentNameResolver implements DocumentNameResolverInterface
{
/** @var array<string, string> */
private array $absoluteUrlCache = [];

/** @var array<string, string> */
private array $canonicalUrlCache = [];

/** @var array<string, bool> */
private array $isAbsoluteCache = [];

/** @var array<string, bool> */
private array $isAbsolutePathCache = [];

/**
* Returns the absolute path, including prefixing '/'.
*
Expand All @@ -31,20 +43,31 @@ final class DocumentNameResolver implements DocumentNameResolverInterface
*/
public function absoluteUrl(string $basePath, string $url): string
{
$uri = BaseUri::from($url);
if ($uri->isAbsolute()) {
return $url;
$cacheKey = $basePath . '|' . $url;
Copy link
Member

@jaapio jaapio Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering how much this would improve. There is not much heavy logic in this method. And I think the actual increase of memory usage does not add up to the small improvement this change has.

A more specify benchmark on this method could help me to understand why this improves the speed.

Copy link
Contributor Author

@CybotTM CybotTM Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BaseUri::from($url) is quite heavy!

absoluteUrl() performance for Documentation-rendertest:

Scenario Calls Time/call Total time Memory Speedup
Old (singleton) 355 3.95 µs 1.40 ms 0 KB -
New (singleton) 355 0.08 µs 0.03 ms +66 KB 53x
Old (new instance) 355 3.97 µs 1.41 ms 0 KB -
New (new instance) 355 2.00 µs 0.71 ms +66 KB 2x

"new instance" means if DocumentNameResolver would use a new instance for every document but it is a Symfony service (singleton).

if (isset($this->absoluteUrlCache[$cacheKey])) {
return $this->absoluteUrlCache[$cacheKey];
}

// Cache URI analysis results separately by URL
if (!isset($this->isAbsoluteCache[$url])) {
$uri = BaseUri::from($url);
$this->isAbsoluteCache[$url] = $uri->isAbsolute();
$this->isAbsolutePathCache[$url] = $uri->isAbsolutePath();
}

if ($this->isAbsoluteCache[$url]) {
return $this->absoluteUrlCache[$cacheKey] = $url;
}

if ($uri->isAbsolutePath()) {
return $url;
if ($this->isAbsolutePathCache[$url]) {
return $this->absoluteUrlCache[$cacheKey] = $url;
}

if ($basePath === '/') {
return $basePath . $url;
return $this->absoluteUrlCache[$cacheKey] = $basePath . $url;
}

return '/' . trim($basePath, '/') . '/' . $url;
return $this->absoluteUrlCache[$cacheKey] = '/' . trim($basePath, '/') . '/' . $url;
}

/**
Expand All @@ -57,8 +80,13 @@ public function absoluteUrl(string $basePath, string $url): string
*/
public function canonicalUrl(string $basePath, string $url): string
{
$cacheKey = $basePath . '|' . $url;
if (isset($this->canonicalUrlCache[$cacheKey])) {
return $this->canonicalUrlCache[$cacheKey];
}

if ($url[0] === '/') {
return ltrim($url, '/');
return $this->canonicalUrlCache[$cacheKey] = ltrim($url, '/');
}

$dirNameParts = explode('/', $basePath);
Expand All @@ -78,6 +106,6 @@ public function canonicalUrl(string $basePath, string $url): string
$urlPass1[] = $part;
}

return ltrim(implode('/', $dirNameParts) . '/' . implode('/', $urlPass1), '/');
return $this->canonicalUrlCache[$cacheKey] = ltrim(implode('/', $dirNameParts) . '/' . implode('/', $urlPass1), '/');
}
}
Loading