Skip to content
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.

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

Parallelize rendering of sibling components to avoid async waterfalls #7071

Merged
merged 8 commits into from
May 18, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { RenderInstruction } from '../types';
import { HTMLBytes, markHTMLString } from '../../escape.js';
import { isPromise } from '../../util.js';
import { renderChild } from '../any.js';
import { EagerAsyncIterableIterator } from '../util.js';

const renderTemplateResultSym = Symbol.for('astro.renderTemplateResult');

Expand Down Expand Up @@ -35,12 +36,16 @@ export class RenderTemplateResult {
async *[Symbol.asyncIterator]() {
const { htmlParts, expressions } = this;

let iterables: Array<AsyncIterable<any>> = [];
for (let i = 0; i < htmlParts.length; i++) {
iterables.push(new EagerAsyncIterableIterator(renderChild(expressions[i])));
}
for (let i = 0; i < htmlParts.length; i++) {
const html = htmlParts[i];
const expression = expressions[i];
const iterable = iterables[i];

yield markHTMLString(html);
yield* renderChild(expression);
yield* iterable;
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions packages/astro/src/runtime/server/render/page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ export async function renderPage(
}
}
}
if (chunk instanceof Response) {
throw new AstroError({
...AstroErrorData.ResponseSentError,
});
}
johannesspohr marked this conversation as resolved.
Show resolved Hide resolved

const bytes = chunkToByteArray(result, chunk);
controller.enqueue(bytes);
Expand Down
76 changes: 76 additions & 0 deletions packages/astro/src/runtime/server/render/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,3 +135,79 @@ export function renderElement(
}
return `<${name}${internalSpreadAttributes(props, shouldEscape)}>${children}</${name}>`;
}

// This eagering consumes an AsyncIterable so that its values are ready to yield out
// ASAP. This is used for list-like usage of Astro components, so that we don't have
// to wait on earlier components to run to even start running those down in the list.
export class EagerAsyncIterableIterator {
#iterable: AsyncIterable<any>;
#queue = new Queue<IteratorResult<any, any>>();
#error: any = undefined;
#next: Promise<IteratorResult<any, any>> | undefined;
constructor(iterable: AsyncIterable<any>) {
this.#iterable = iterable;
this.#run();
}

async #run() {
let gen = this.#iterable[Symbol.asyncIterator]();
let value: IteratorResult<any, any> | undefined = undefined;
do {
this.#next = gen.next();
try {
value = await this.#next;
this.#queue.push(value);
} catch (e) {
this.#error = e;
}
} while (value && !value.done);
}

async next() {
if (this.#error) {
throw this.#error;
}
if (!this.#queue.isEmpty()) {
return this.#queue.shift()!;
}
await this.#next;
return this.#queue.shift()!;
johannesspohr marked this conversation as resolved.
Show resolved Hide resolved
}

[Symbol.asyncIterator]() {
return this;
}
}

interface QueueItem<T> {
item: T;
next?: QueueItem<T>;
}

/**
* Basis Queue implementation with a linked list
*/
class Queue<T> {
head: QueueItem<T> | undefined = undefined;
tail: QueueItem<T> | undefined = undefined;

push(item: T) {
if (this.head === undefined) {
this.head = { item };
this.tail = this.head;
} else {
this.tail!.next = { item };
this.tail = this.tail!.next;
}
}

isEmpty() {
return this.head === undefined;
ematipico marked this conversation as resolved.
Show resolved Hide resolved
}

shift(): T | undefined {
const val = this.head?.item;
this.head = this.head?.next;
return val;
}
}
12 changes: 12 additions & 0 deletions packages/astro/test/fixtures/parallel/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"name": "@test/parallel-components",
"version": "0.0.0",
"private": true,
"scripts": {
"build": "astro build",
"dev": "astro dev"
},
"dependencies": {
"astro": "workspace:*"
}
}
11 changes: 11 additions & 0 deletions packages/astro/test/fixtures/parallel/src/components/Delayed.astro
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
const { ms } = Astro.props
const start = new Date().valueOf();
await new Promise(res => setTimeout(res, ms));
const finished = new Date().valueOf();
---
<section>
<h1>{ms}ms Delayed</h1>
<span class="start">{ start }</span>
<span class="finished">{ finished }</span>
</section>
8 changes: 8 additions & 0 deletions packages/astro/test/fixtures/parallel/src/pages/index.astro
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
import Delayed from '../components/Delayed.astro'
---

<Delayed ms={30} />
<Delayed ms={20} />
<Delayed ms={40} />
<Delayed ms={10} />
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import Wait from '../components/Wait.astro';
<p>Section content</p>
</Wait>
<h2>Next section</h2>
<Wait ms={50}>
<Wait ms={60}>
<p>Section content</p>
</Wait>
<p>Paragraph 3</p>
Expand Down
24 changes: 24 additions & 0 deletions packages/astro/test/parallel.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { expect } from 'chai';
import * as cheerio from 'cheerio';
import { loadFixture } from './test-utils.js';

describe('Component parallelization', () => {
let fixture;

before(async () => {
fixture = await loadFixture({
root: './fixtures/parallel/',
});
await fixture.build();
});

it('renders fast', async () => {
let html = await fixture.readFile('/index.html');
let $ = cheerio.load(html);

let firstStart = Number($('.start').first().text());
let lastStart = Number($('.start').last().text());
let timeTook = lastStart - firstStart;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

I think the test should measure the longest time difference instead.
Screenshot (158)

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

@lilnasy lilnasy May 12, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

expect(timeTook).to.be.lessThan(50, 'the components in the list were kicked off more-or-less at the same time.');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 👍

});
});
6 changes: 6 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.