Parallelize rendering of sibling components to avoid async waterfalls

[johannesspohr]

Changes

This changes the rendering behavior of Astro components to render child components in parallel.
In cases where multiple components in different subtrees fetch data, this data fetching can be done in parallel and will improve loading times in those scenarios.

Testing

A test is added to confirm the parallel execution of the artificial delays. Also, an existing test (number of stream chunks) had to be changed, because the parallel execution lead to 2 chunks instead of the expected 3.

Docs

Docs & changeset will be added as this is just a draft

[changeset-bot]

🦋 Changeset detected

Latest commit: 663cdd6

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

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

[lilnasy]

I know this was written this way in the original PR as well, but this message will be shown if the test fails.

[johannesspohr]

Thanks for the heads up, I've seen both interpretations (putting the expected behaviour or the error message in there), but I'm happy to make it more explicit 👍

[lilnasy]

This addresses a major concern. Thanks for working on this.

[lilnasy]

All components are kicked off at the same time, so timeTook here will be always be 0 or 1.

I think the test should measure the longest time difference instead.

Suggested change

	let firstStart = Number($('.start').first().text());
	let lastStart = Number($('.start').last().text());
	let timeTook = lastStart - firstStart;
	const startTimes = Array.from($('.start')).map(element => Number(element.children[0].data))
	const finishTimes = Array.from($('.finished')).map(element => Number(element.children[0].data))
	const timeTook = Math.max(...finishTimes) - Math.min(...startTimes);

[johannesspohr]

For my understanding, the fact that all kick off at the same time is the result of this change. Otherwise, they would render in order and the start times would differ by more than 50ms. The big threshold of 50ms is a bit misleading though (I just copied that from the old pr as well)

[lilnasy]

Yeah I agree that the number is misleading. It's what made me think the intention is to measure the render time. Maybe the duration limit could be shortened and renamed to kickedOffWithin?

[johannesspohr]

I also liked your idea, so I added the check as well. It's maybe not the cleanest unit test, as it tests more than one thing, but I think it's still OK.

[johannesspohr]

I changed the behavior a bit to not start buffering right away, but only when an async component is reached (e.g. the event loop picks up the setTimeout). This could be optimized even further if we had a way to know whether a subcomponent tree has any async components in it, but for now, the current implementation should at least not cause any impact for components, that only have children that resolve immediately.

I ran benchmarks with autocannon against the portfolio example:

• before this PR: 204 req / s
• with caching all eagerly: 168 / s
• with current solution: 199 req / s

[smnbbrv]

Thank you very much, when this gets merged the world becomes a lot better! This would close the discussion withastro/roadmap#577

This could be optimized even further if we had a way to know whether a subcomponent tree has any async components in it, but for now, the current implementation should at least not cause any impact for components, that only have children that resolve immediately.

If I get it right, you mean every async subcomponent would have its own setTimeout as of now (and this is not optimal)? Or this actually fixes only top-level async components?

[johannesspohr]

If I get it right, you mean every async subcomponent would have its own setTimeout as of now (and this is not optimal)? Or this actually fixes only top-level async components?

To add a bit of context: the generic rendering operation is async, so it can either return a promise, or return an immediately resolved promise, that will basically synchronously continue the code execution. Caching a resolved promise is not only superfluous, but adds performance overhead, as we (synchronously) write everything into a buffer, just to read it again later, but we could have as well just streamed the output to the client.

In this scenario, we don't know whether a rendering operation will immediately resolve or not. So I opted for a middleground. We will instantiate the EagerAsyncIterableIterator for all children (because we won't know if an iterator started otherwise), but they are set to operate in pass-through / non-buffered mode. This means, that they will simply forward the next() call to the inner iterator and cause minimal impact. Now we register the timeout, which will run as soon as a child iterator does something actually asynchronous. Only then will the next "tick" be available and we have some slack time to start rendering the other children. So we will check which iterators haven't started yet, and start buffering those. In an ideal world, we could skip buffering the iterators that are also synchronous, as we gain nothing from buffering those (streaming the output directly is as fast). But other children, which are actually async, will start doing their stuff (network requests or similar).

Say the layout has 4 components:

• Header resolves immediately
• Content fetches some data
• SeoContent resolves immediately
• Footer fetches some data

We start by wrapping all in the EagerAsyncIterableIterator, but don't buffer yet, so nothing actually starts rendering.

• The Header gets rendered, as it synchronously resolves, it gets directly streamed to the output
• Content is rendered, but it needs to wait for the data, so it is async
• Then the setTimeout fires, which starts buffering all the iterators that haven't started, so SeoContent and Footer
• SeoContent resolves immediately, so we write that into the buffer (this step is useless, but we have no way of telling afaik)
• Footer is buffered, so it fires its network request
• Notice that at this point, we have the network requests for both async components running in parallel, the only delay is waiting for the tick and rendering the SeoContent, but this is not avoidable without knowing the async style
• When the Content resolves, the result will be streamed to the client
• The SeoContent is immediately streamed from the buffer
• The Footer is either directly streamed from the cache (if the network request resolved already) or streamed once its ready.

I hope this explains the process and also why the setTimeout is needed. There might be a better way to do it, but due to my limited knowledge about iterators etc., I don't see any.

Note also that this happens recursively in every child component.

[johannesspohr]

@ematipico Thanks for the detailed review. I added more documentation. I find it kinda hard to estimate what is understandable to somebody who didn't think about this for hours 😅 so please keep bugging me if there still is something not clear from the comments.

[bluwy]

On top of Ema's review, I did some perf runs using wrk with the portfolio example, and didn't notice any regressions or improvements, which I think is a good thing as it should be more noticable for async tasks that runs longer.

I guess a tiny concern is that running many async stuff in parallel could pressure the CPU and memory a bit, but I think in practice pages/components wouldn't be doing many intensive stuff at once.

[johannesspohr]

I guess a tiny concern is that running many async stuff in parallel could pressure the CPU and memory a bit, but I think in practice pages/components wouldn't be doing many intensive stuff at once.

True, it might a bit of overhead to the internal event loop etc. as we need to keep track of the different rendering states, but still JavaScript is single-threaded, so it can't really do things in parallel. It can only do other stuff while we're waiting for external I/O (mostly network I assume).

[matthewp]

@johannesspohr It seems like we are pretty close on this one. We are doing the 2.5 release tomorrow. If you think you can address the review comments this could possibly go out then. Otherwise 2.6 will be in a couple of weeks.

[johannesspohr]

@matthewp I was able to resolve everything except for that one thing where I'm unsure if this needs further documentation or tests. However, from a functionality standpoint, we should be good to go.

[ematipico]

Thank you for addressing my pedantic feedback! 😅 💪 🚀

[matthewp]

Thanks so much @johannesspohr, happy to have this in.

[smnbbrv]

Hi guys, just tested the solution, it works for single components. However, it doesn't for fragment arrays, here is the follow-up issue #7127