perf: avoid repeated sidebar subtree scans

[ematipico]

Description

The sidebar now finds all groups containing the current page in one traversal. Previously, each group repeatedly flattened its subtree, causing unnecessary work for deeply nested sidebars.

Should save some memory and a bit of milliseconds.

[changeset-bot]

🦋 Changeset detected

Latest commit: cfdba20

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@astrojs/starlight	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[netlify]

✅ Deploy Preview for astro-starlight ready!

Name	Link
🔨 Latest commit	cfdba20
🔍 Latest deploy log	https://app.netlify.com/projects/astro-starlight/deploys/6ab50978edac830008a47940
😎 Deploy Preview	https://deploy-preview-4149--astro-starlight.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 100 (no change from production) Accessibility: 100 (no change from production) Best Practices: 100 (no change from production) SEO: 100 (no change from production) PWA: - View the detailed breakdown and full score reports
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[astrobot-houston]

size-limit report 📦

Path	Size
/index.html	6.1 KB (0%)
/guides/example/index.html	6.04 KB (0%)
/_astro/*.js	25.43 KB (0%)
/_astro/*.css	14.72 KB (0%)

[delucis]

I wonder if instead of optimising this here we should add some logic in navigation.ts to tag groups as current when building the sidebar? We’re already marking current leaf nodes in the tree there, but I guess we could also mark the ancestors of the current item at the same time?

This would have a couple of advantages:

1. It could be faster because we could do it inside some existing iterations.
2. It means the logic is abstracted away from the rendering component so users who reimplement the component don’t also need to reimplement this.

There is a small risk though: if a user for some reason adds/changes the current page link in route middleware, they would be responsible for marking ancestors at the same time because the component would not do it for them any more. Similarly, if the current page link was removed, they’d need to remove the current marker from the containing folders too. I’m not sure how common those scenarios are to evaluate the risk–benefit trade-off. Might need some more feedback to decide for sure.

(A third option would be to have a new layer of Starlight logic that runs after user middleware but before rendering to do work like this, but I’m not sure that’s a good idea because the middleware is kind of the escape hatch for changing Starlight behaviour, so to enforce stuff on top of it is potentially undesirable.)

[ematipico]

There's the risk that if a user uses the route middleware to change the sidebar, the information will become stale and could cause some undesired behaviour

[codspeed]

Merging this PR will improve performance by 41.49%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
✅ 2 untouched benchmarks

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
⚡	sidebar current groups	13.2 ms	9.3 ms	+41.49%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.

---

_{Comparing perf/sidebar-current-groups (cfdba20) with main (fafea30)}

[delucis]

OK, thinking about this some more, I think the code does need to run at this level — it’s essentially a rendering concern not a data one. So running it in the component makes sense.

I do think it would be nice to keep the behaviour a bit more reusable if someone is overriding the component. I considered a utility attached to Astro.locals.starlightRoute, which could be lazily initialised for users to simplify the API and not require prop drilling:

<details open={Astro.locals.sidebarEntryHasCurrent(entry) || !entry.collapsed}>

But I’m not 100% sure about introducing functions on the route data object as it’s not something we’ve done until now and there may be gotchas. It would be the cleanest for users though.

[HiDeoo]

While thinking about Chris's comments, I looked at other places where we recursively walk the sidebar on every page. These should definitely be measured for performance improvements as well, but maybe these other places could also benefit from similar optimizations.

• flattenSidebar: push into a single shared array/accumulator instead of using flatMap() at each level
• getSidebarHash + recursivelyBuildSidebarIdentity: push labels and hrefs into a single shared array/accumulator and use a single join('') at the end

Now to get back to Chris's comments:

I do think it would be nice to keep the behaviour a bit more reusable if someone is overriding the component.

While true, and if I'm not missing anything, I don't see any change in the new implementation that would require us to do so at the same time? Nothing would change for people overriding the sidebar and re-using the publicly exported <SidebarSublist> component (which is not overridable). The new prop is optional, and the new set is computed if it's missing. <SidebarSublist sublist={...} /> in a custom sidebar would behave exactly as before I think?

I guess the idea would be to make it easier to write custom <SidebarSublist>? Altho that's not a regression as flattenSidebar() was never exported?

I considered a utility attached to Astro.locals.starlightRoute

Adding a function to the route data could be a bit scary. One potential issue coming to mind is that anything that serialises or copies the route data could break.

which could be lazily initialised

If I understand correctly, I guess this could be tricky to get right, e.g. what if a middleware calls the helper and then mutates the sidebar?

If we were to rely on a helper using starlightRoute.sidebar as source of truth, a component override could also transform the sidebar before rendering it, e.g. mapping and spreading objects, filtering nested entries, or even building its own sidebar, which could break object identity and cause unexpected behavior for the current page's group?

The flag on groups, computed after route middleware, I guess could work, but it's definitely a new kind of route-data field, readable in components but not during middleware. Tricky to document and reason about I would guess.

I probably need to think more about it but without too much thinking, exported helpers sound way less scary at first glance (but still a new "concept" in Starlight (but less difficult to reason about compared to the flag on groups)). It could be a sidebarGroupHasCurrent(group) taking a group as parameter and relying on a WeakMap to avoid recomputing the same group multiple times. It could also be a helper similar to the current PR implementation, e.g. getSidebarCurrentGroups(sidebar) returning a set of groups containing the current page and it's up to the user to pass it down (altho I guess it doesn't remove prop drilling like the previous approach). I guess Starlight could even use the same helper in its own sublist.

[delucis]

Thanks for the feedback and thoughts @HiDeoo! OK, in that case probably an exported helper — like flattenSidebar() — is the “easy win” version for now. I can clean this PR up to do that.

that's not a regression as flattenSidebar() was never exported?

Correct. I was mostly thinking how we get the complaint sometimes from people reimplementing components in overrides that they can’t access utilities imported in the original implementation (i.e. they want to be able to copy/paste our implementation, then modify). My general approach has been to try to hoist as much data processing up into route data as possible, so that our components can be focused on presentation and are a bit easier to reuse or reference. But this is definitely hard in some cases like this one where you want to support data mutation and the current group is a derived state.

So in theory, standardising and exporting utilities like this one day could be a solution. But not one we have to take today.

If I understand correctly, I guess this could be tricky to get right, e.g. what if a middleware calls the helper and then mutates the sidebar?

Yeah, the utility would only be really designed for use in a component/after mutations are done, so tricky for sure.

---

The other — more disruptive — change could be to hide the sidebar data structure behind custom APIs that take care of mutating the tree instead of interacting directly with the underlying arrays of entries. That way current state could be flagged initially and then managed by those APIs. But moving away from plain JS data structures and the flexibility that gives is not the easiest thing to design for without limiting what people can do. Essentially you would want these behaviours I guess:

• setting entry.isCurrent should propagate up the tree, marking parents
• inserting or removing a sub-tree that includes { isCurrent: true } among children should propagate up the tree

For the set/remove cases that’s just walking up the chain of parents, which could be quite cheap to do. For the sub-tree insertion case, you need to dive into the new sub-tree to find if there is a current child before walking up.

But in any case, that’s probably not justified for now. Maybe if in some future we have more complex sidebar data structures to handle multiple sidebars etc. we could find ourselves wanting to help users with such an API and could do this then.

[ematipico]

The other — more disruptive — change could be to hide the sidebar data structure behind custom APIs that take care of mutating the tree instead of interacting directly with the underlying arrays of entries

I believe this is a good comprise. It provides enough customisation, while being protective enough of the internal Starlight infra, so that it's possible to change the structure as we see fit.

FYI Nimbus added something similar https://nimbus-docs.com/navigation/sidebar/#transform-the-final-tree
It has a different API than starlight, but maybe it can be used as source of inspiration

[delucis]

As suggested by @HiDeoo I switched the implementation to use a WeakMap and moved the new implementation into an imported utility to make it potentially easier to expose one day.

So now the component can just call sidebarGroupHasCurrent(group) and it will only scan the tree once per page without the need for passing down current groups with prop drilling. I guess in theory this means that under memory pressure, the WeakMap could get garbage collected leading to a recomputation during a page render, but that seems like a rare occurrence?

Now this is in a separate utility I think we could probably benchmark it more easily so will take a look after lunch at writing a before/after comparison.

[delucis]

~3x speed-up:

name          hz     min     max    mean     p75     p99    p995    p999     rme  samples
old   120,629.54  0.0078  2.5025  0.0083  0.0083  0.0094  0.0103  0.0270  ±0.99%    60315
new   365,587.14  0.0025  0.1273  0.0027  0.0028  0.0034  0.0036  0.0067  ±0.17%   182794

new - 3.03x faster than old

Tested this locally in our existing functional benchmark file with a small function to mock the recursive rendering.

Benchmark code

function mockSidebarRender(sublist: SidebarEntry[], getOpen: (entry: SidebarGroup) => boolean) {
	sublist.map((entry) => {
		if (entry.type !== 'link') {
			getOpen(entry);
			mockSidebarRender(entry.entries, getOpen);
		}
	});
}

describe('sidebar current groups', () => {
	bench('old', () => {
		mockSidebarRender(getSidebar(context.url.pathname, route.locale), (entry) =>
			flattenSidebar(entry.entries).some((i) => i.isCurrent)
		);
	});
	bench('new', () => {
		mockSidebarRender(getSidebar(context.url.pathname, route.locale), sidebarGroupHasCurrent);
	});
});

I’ll add just the new one to this PR so we test it in CI too.

---

Edit: I will note that I think real-world impacts may be small. For example, if I complicate the “mock” render slightly more to mimic a bit some of the other attributes and stuff we do in the <SidebarSublist> component, the difference between the two algorithms reduces:

name          hz     min     max    mean     p75     p99    p995    p999     rme  samples
old    63,699.57  0.0143  0.6598  0.0157  0.0155  0.0219  0.0297  0.0680  ±0.40%    31850
new   102,137.86  0.0090  0.0995  0.0098  0.0098  0.0125  0.0145  0.0547  ±0.22%    51069

new - 1.60x faster than old

Updated mock render function

function mockSidebarRender(sublist: SidebarEntry[], getOpen: (entry: SidebarGroup) => boolean) {
	sublist.map((entry) => {
		if (entry.type === 'link') {
			return {
				href: entry.href,
				'aria-current': entry.isCurrent ? 'page' : undefined,
				...entry.attrs,
				children: [entry.label, entry.badge],
			};
		} else {
			return {
				open: getOpen(entry) || !entry.collapsed,
				children: [mockSidebarRender(entry.entries, getOpen), entry.label, entry.badge],
			};
		}
	});
}

And none of this is accounting for actual render time on Astro’s side, so I guess it’ll be more of a marginal gain. ~16 µs => ~10 µs per page, so a gain of ~6 µs (which is consistent between both benchmarks). That means you need 150k+ pages to see a 1 second speed-up maybe?

But it all needs caveats because these micro-benchmarks may not mirror real-world performance behaviour. For example, maybe the flattenSidebar() approach here is less well optimised in the real world when calls are separated by more rendering work or something? And this isn’t measuring memory impact either.

[HiDeoo]

Not quite sure if this was ready for review or not so I went ahead and submitted one anyway, I can still do a new review later if needed.

The changes looks good and make sense to me. Like you mentioned, I expect this to be pretty marginal in terms of impact.

Note that this would need a changeset once ready for release.

Once merged, I'll investigate the change suggestions I made and evaluate if they are worth it or not as a follow-up.

[delucis]

OK, I added a changeset and I think this ready for a final (hopefully) review!

I backported the benchmark onto main so we get a comparison from Codspeed too, and it’s reporting an improvement, although unfortunately with the dreaded “different runtimes” warning, so not so helpful maybe. But I think we’re still confident that this is an improvement based on local testing as well.

[HiDeoo]

Amazing, thanks both for the improvements 🙌 🔥

Once released, I'll update #4180 to add the new benchmark, and once we can merge it, it should hopefully make the benchmarks a little bit more reliable.

[ematipico]

Another amazing improvement 🚀

[delucis]

Thanks for the initial investigation and implementation @ematipico 🙌