Batch iterator buffering to reduce number of timeout calls

[johannesspohr]

Changes

• What does this change?
  This batches iterator buffering to solve Over 10k setTimeout() on Route Rendering #7460

Testing

Difficult due to inability to count the number of cals to setTimeout. Will be tackled in a different PR.

Docs

Changeset is included

[changeset-bot]

🦋 Changeset detected

Latest commit: 7214b1b

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

[natemoo-re]

!bench

[github-actions]

PR Benchmark

Memory

	Elapsed time (s)	Memory used (MB)	Final memory (MB)
SSR build	4.44	90.33	129.96
Client build	0.13	0.32	129.99
Static generate	0.73	5.91	135.17

Render

Page	Avg (ms)	Stdev (ms)	Max (ms)
/astro	1.61	3.03	31.47
/md	6.76	4.71	52.25
/mdx	172.62	33.07	323.40

Server stress

	Avg	Stdev	Max
Latency	2019.75 ms	337.68 ms	3578 ms

	Avg	Stdev	Min	Total in 30s
Req/Sec	479.47	158.97	422	14.4k requests
Bytes/Sec	34.1 MB	11.3 MB	30 MB	1.02 GB read

Main Benchmark

Memory

	Elapsed time (s)	Memory used (MB)	Final memory (MB)
SSR build	4.32	90.55	130.05
Client build	0.12	0.38	130.04
Static generate	0.68	5.73	135.17

Render

Page	Avg (ms)	Stdev (ms)	Max (ms)
/astro	4.35	4.76	41.93
/md	6.45	3.22	36.93
/mdx	171.97	33.91	363.38

Server stress

	Avg	Stdev	Max
Latency	2071.74 ms	427.89 ms	3834 ms

	Avg	Stdev	Min	Total in 30s
Req/Sec	467.54	180.11	177	14.0k requests
Bytes/Sec	33.2 MB	12.8 MB	12.6 MB	997 MB read

[itsmatteomanf]

I ran the same benchmarks against my own PR which added the queueMicrotask() implementation, everything here seems either equal or, more often, better than that case.

#7494 (comment)

To me this looks good. Might be worth pushing soon, to catch the next release to revert the performance regression.

[bluwy]

I think this PR is in a ready state as the benchmark are sort-of the test, and it does show a big perf improvement! Rendering .astro files take avg 1.6ms from 4.3ms.

[itsmatteomanf]

It had to get worse to get better, but it might be a nice performance improvement. Once it’s actually released I’ll compare with my massive page that caused the issues.
v2.7.1 vs. v2.7.2 vs. this one.

[johannesspohr]

Thanks for the feedback everybody, I'd like to have a proper test that everything works as expected, but I didn't find a good solution to spy on setTimeout. Any ideas how to test this? (I guess we could import / inject setTimeout somehow to replace it with a counting function)

I also threw in a couple of good ol' console.logs and saw the number of setTimeouts drop from 14 to 7 with the new version. So I assume it works, but it's really hard to really track what's going on and how many calls it should be.

[itsmatteomanf]

but it's really hard to really track what's going on and how many calls it should be.

Yeah, I had like 3k components which triggered over 10k, not sure how, why or when. I traced it to that function, but actually counting them was hard. To me a reduction sounds good, as long as the actual rendering works correctly, like it should.

queueMicrotask() is slower than setTimeout(), that we figured out, and I'm not knowledgeable enough on Astro's codebase to say if there is a better way. I'd assume not, but improvements can always be done, usually ahah

I'd like to have a proper test that everything works as expected

What are you worried about not working?

[johannesspohr]

I mostly wanted to verify that the batching is in fact happening and make this expectation explicit with a test, so future changes won't break it (as it's kind of implicit at the moment). So if everybody is OK with it we can merge it for now, and try adding some regression tests in a follow-up PR.

[itsmatteomanf]

Fine by me, I feel like at the very least resolving the perf regression is a nice thing. This should bring performance even higher than it was. Testing wise I’d have to think about it.

[bluwy]

I'm still a bit worried having a timing-based test on this. When we added the parallel rendering timing tests, it became flaky now and then that we had to bump the timeout. But if you can figure out a way that doesn't cause flakiness, that would be great!