-
Notifications
You must be signed in to change notification settings - Fork 394
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!: use larger buffers for blob_io and ephemeral_file #7485
Merged
problame
merged 35 commits into
main
from
problame/larger-buffers-io/perf-use-larger-buffers-in-blob_io-and_ephemeral_file
Apr 26, 2024
Merged
perf!: use larger buffers for blob_io and ephemeral_file #7485
problame
merged 35 commits into
main
from
problame/larger-buffers-io/perf-use-larger-buffers-in-blob_io-and_ephemeral_file
Apr 26, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rs-io/refactor-buffered-writer
…ver underlying writer
…op cache-on-write
2 tasks
2796 tests run: 2675 passed, 0 failed, 121 skipped (full report)Flaky tests (2)Postgres 15
Postgres 14
Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
42bb9cd at 2024-04-26T11:41:43.782Z :recycle: |
…r' into problame/larger-buffers-io/refactor-ephemeral-file-reuse-buffered-io
…e-buffered-io' into problame/larger-buffers-io/perf-use-larger-buffers-in-blob_io-and_ephemeral_file
…rs-io/refactor-buffered-writer
…to problame/larger-buffers-io/refactor-size-tracking-writer
…r' into problame/larger-buffers-io/refactor-ephemeral-file-reuse-buffered-io
…or-ephemeral-file-reuse-buffered-io' into problame/larger-buffers-io/refactor-ephemeral-file-reuse-buffered-io--with-write-path-prewarming
…rs-io/refactor-buffered-writer
…to problame/larger-buffers-io/refactor-size-tracking-writer
…r' into problame/larger-buffers-io/refactor-ephemeral-file-reuse-buffered-io
…e-buffered-io' into problame/larger-buffers-io/refactor-ephemeral-file-reuse-buffered-io--with-write-path-prewarming
…e-buffered-io--with-write-path-prewarming' into problame/larger-buffers-io/perf-use-larger-buffers-in-blob_io-and_ephemeral_file
72f9904
to
bfcba3f
Compare
…e-buffered-io--with-write-path-prewarming' into problame/larger-buffers-io/perf-use-larger-buffers-in-blob_io-and_ephemeral_file
…/refactor-ephemeral-file-reuse-buffered-io--with-write-path-prewarming refactor(ephemeral_file): bring back pre-warming of PS page cache on write
…tor-ephemeral-file-reuse-buffered-io' into problame/larger-buffers-io/perf-use-larger-buffers-in-blob_io-and_ephemeral_file
…rs-io/refactor-buffered-writer
Base automatically changed from
problame/larger-buffers-io/refactor-ephemeral-file-reuse-buffered-io
to
main
April 26, 2024 11:01
…rs-io/refactor-buffered-writer
…to problame/larger-buffers-io/perf-use-larger-buffers-in-blob_io-and_ephemeral_file
jcsp
approved these changes
Apr 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
part of #7124
Problem
(Re-stating the problem from #7124 for posterity)
The
test_bulk_ingest
benchmark shows about 2x lower throughput withtokio-epoll-uring
compared tostd-fs
.That's why we temporarily disabled it in #7238.
The reason for this regression is that the benchmark runs on a system without memory pressure and thus std-fs writes don't block on disk IO but only copy the data into the kernel page cache.
tokio-epoll-uring
cannot beat that at this time, and possibly never.(However, under memory pressure, std-fs would stall the executor thread on kernel page cache writeback disk IO. That's why we want to use
tokio-epoll-uring
. And we likely want to use O_DIRECT in the future, at which point std-fs becomes an absolute show-stopper.)More elaborate analysis: https://neondatabase.notion.site/Why-test_bulk_ingest-is-slower-with-tokio-epoll-uring-918c5e619df045a7bd7b5f806cfbd53f?pvs=4
Changes
This PR increases the buffer size of
blob_io
andEphemeralFile
from PAGE_SZ=8k to 64k.Longer-term, we probably want to do double-buffering / pipelined IO.
Resource Usage
We currently do not flush the buffer when freezing the InMemoryLayer.
That means a single Timeline can have multiple 64k buffers alive, esp if flushing is slow.
This poses an OOM risk.
We should either bound the number of frozen layers (#7317).
Or we should change the freezing code to flush the buffer and drop the allocation.
However, that's future work.
Performance
(Measurements done on i3en.3xlarge.)
The
test_bulk_insert.py
is too noisy, even with instance storage. It varies by 30-40%. I suspect that's due to compaction. Raising amount of data by 10x doesn't help with the noisiness.)So, I used the
bench_ingest
from @jcsp 's #7409 .Specifically, the
ingest-small-values/ingest 128MB/100b seq
andingest-small-values/ingest 128MB/100b seq, no delta
benchmarks.The
8k
is from before this PR, the64k
is with this PR.The values are the throughput reported by the benchmark (MiB/s).
We see that this PR gets
tokio-epoll-uring
from 67% to 87% ofstd-fs
performance in theseq
benchmark. Notably,seq
appears to hit some other bottleneck at55 MiB/s
. CC'ing #7418 due to the apparent bottlenecks in writing delta layers.For
seq, no delta
, this PR getstokio-epoll-uring
from 64% to 91% ofstd-fs
performance.