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

perf(pageserver): postpone vectored get fringe keyspace construction #7904

Merged
merged 1 commit into from
May 30, 2024

Conversation

skyzh
Copy link
Member

@skyzh skyzh commented May 29, 2024

Problem

Perf shows a significant amount of time is spent on Keyspace::merge. This pull request postpones merging keyspace until retrieving the layer, which contributes to a 30x improvement in aux keyspace basebackup time.

--- old
10000 files found in 0.580569459s
--- new
10000 files found in 0.02995075s

Summary of changes

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

@skyzh skyzh requested a review from a team as a code owner May 29, 2024 15:41
@skyzh skyzh requested review from jcsp and VladLazar May 29, 2024 15:41
Copy link

3144 tests run: 3011 passed, 0 failed, 133 skipped (full report)


Flaky tests (3)

Postgres 16

  • test_crafted_wal_end[last_wal_record_xlog_switch_ends_on_page_boundary]: debug
  • test_wal_restore_initdb: release
  • test_wal_restore: release

Code coverage* (full report)

  • functions: 31.4% (6490 of 20663 functions)
  • lines: 48.4% (50224 of 103736 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
d605bd1 at 2024-05-29T16:32:17.922Z :recycle:

Copy link
Contributor

@jcsp jcsp left a comment

Choose a reason for hiding this comment

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

LGTM

(CC @VladLazar so that this is in your inbox when you're back in the office.)

@skyzh skyzh merged commit 33395dc into main May 30, 2024
64 checks passed
@skyzh skyzh deleted the skyzh/basebackup-impr branch May 30, 2024 14:31
@skyzh
Copy link
Member Author

skyzh commented May 30, 2024

On staging the basebackup time drops to a reasonable number (30s->2s for now). With the delete tombstone pull request it should be even faster.

a-masterov pushed a commit that referenced this pull request Jun 3, 2024
…7904)

Perf shows a significant amount of time is spent on `Keyspace::merge`.
This pull request postpones merging keyspace until retrieving the layer,
which contributes to a 30x improvement in aux keyspace basebackup time.

```
--- old
10000 files found in 0.580569459s
--- new
10000 files found in 0.02995075s
```

Signed-off-by: Alex Chi Z <chi@neon.tech>
@@ -318,7 +318,7 @@ pub(crate) struct LayerFringe {
#[derive(Debug)]
struct LayerKeyspace {
layer: ReadableLayer,
target_keyspace: KeySpace,
target_keyspace: Vec<KeySpace>,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: making this member a KeySpaceRandomAccum would have been more expressive in my opinion, but this is fine.

@VladLazar
Copy link
Contributor

VladLazar commented Jun 10, 2024

Looks good to me as well. Nice find, Chi!

skyzh added a commit that referenced this pull request Jun 14, 2024
follow up on #7904

avoid a layer of indirection introduced by `Vec<Range<Key>>`

Signed-off-by: Alex Chi Z <chi@neon.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants