feat(page_service): timeout-based batching of requests

[VladLazar]

Problem

We don't take advantage of queue depth generated by the compute
on the pageserver. We can process getpage requests more efficiently
by batching them.

Summary of changes

Batch up incoming getpage requests that arrive within a configurable time window (server_side_batch_timeout).
Then process the entire batch via one get_vectored timeline operation.
By default, no merging takes place.

Testing

• Functional: page_service: getpage batching: refactor & minor fixes #9792
• Performance: will be done in staging/pre-prod

[github-actions]

5490 tests run: 5264 passed, 0 failed, 226 skipped (full report)

---

Code coverage* (full report)

• functions: 31.5% (7932 of 25181 functions)
• lines: 49.6% (62944 of 126880 lines)

* collected from Rust tests only

---

_{The comment gets automatically updated with the latest test results
2bea398 at 2024-11-18T20:25:20.479Z :recycle:}

[problame]

The terminology is 50% debounce 50% batching.

I think batching is better, let's adjust the names.

As a person who doesn't mind wordy names, page_service_server_side_batching is how I'd call the functionality / code module.

Maybe server_side_batch_timeout for the config flag?

And BatchedFeMessageS instead of DebouncedFeMessage for the type?

---

During the hackathon, and now, I wonder(ed) whether we could refactor the batching loop into a Stream / $itertoolsadapter

Like, at the start of handle_pagerequests, split the pgb into reading and writing end (maybe fake that using Arc<Mutex<Pgb>>).
The wrap the reading end into the thing that implements Stream, spitting out DebouncedFeMessage as the strema item.

Orthogonal to that, can we please move the batching into a helper function so the high-level control flow of handle_pagerequests remains clear-ish?

---

I flagged a couple of places that warn! aggressively.
I actually don't know if we log getpage errors. If we do, then I think it's fine to just leave these warn! messages & dismiss the conversation.

[VladLazar]

The terminology is 50% debounce 50% batching.

I think batching is better, let's adjust the names.

As a person who doesn't mind wordy names, page_service_server_side_batching is how I'd call the functionality / code module.

Maybe server_side_batch_timeout for the config flag?

And BatchedFeMessageS instead of DebouncedFeMessage for the type?

You're right. I'll touch up the names.

During the hackathon, and now, I wonder(ed) whether we could refactor the batching loop into a Stream / $itertoolsadapter

Like, at the start of handle_pagerequests, split the pgb into reading and writing end (maybe fake that using Arc<Mutex<Pgb>>). The wrap the reading end into the thing that implements Stream, spitting out DebouncedFeMessage as the strema item.

I also wondered about this while going through the code, but resisted the temptation to rewrite.
Let's create an epic and add a follow-up in there.

Orthogonal to that, can we please move the batching into a helper function so the high-level control flow of handle_pagerequests remains clear-ish?

Sure, I'll give it a go.

[VladLazar]

• Switched naming to batching
• Looked into logging. We already log on both pagestream and ingest flows, so I've removed the redundant logging.
• Reworked response filling to work around one copy
• Refactored batching into a separate method.

[problame]

I see a lot of plumbing-through of the server_side_batch_timeout variable.
Personally I wouldn't mind just plumbing through the &'static PageServerConf.
Or building a PageServiceConf object out of PageServerConf.
Easily punt-able though.

---

I think the only two things I'm hard-requesting-changes for is the use of smallvec<1> for the return value of read_batch_from_connection, and the elimination of the panic!(unsupported)