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

Deprecation of SetupForCompaction causes slow compactions #787

Closed
Yuval-Ariel opened this issue Dec 24, 2023 · 1 comment · Fixed by #788
Closed

Deprecation of SetupForCompaction causes slow compactions #787

Yuval-Ariel opened this issue Dec 24, 2023 · 1 comment · Fixed by #788
Assignees
Labels
bug Something isn't working compaction performance

Comments

@Yuval-Ariel
Copy link
Contributor

Yuval-Ariel commented Dec 24, 2023

Describe the bug
During the rebase on 8.6.7 (#736) we've encountered very long stalls as seen below:
image
Where purple is the rebased branch and light blue is the current main.

Further investigation showed that the stalls happened since the compaction ran much slower. Running iostat during that time showed that in the rebased version, the reads from disk are consistently at 4kb (rareq-sz) and no reads are queued (rrqm/s).
We've tried playing with compaction_readahead_size as written in https://smalldatum.blogspot.com/2023/11/debugging-perf-changes-in-rocksdb-86-on.html but all values showed some degradation.

Release 2.7 of Speedb is based on RocksDB 8.1.1 and there, the compaction reads are using the filesystem defaults ('compaction_readahead_size' is 0 by default) for prefetching which results in much better performance than using any compaction_readahead_size value with the rebased version (rebased on Rocksdb 8.6.7). Some of this change was added in facebook/rocksdb#11631.

The reason for the slower performance of the compaction read speed in the rebased version when using compaction_readahead_size = 0 (using the OS page cache) is because of this PR facebook/rocksdb#11658, which removes hinting the FS with POSIX_FADV_NORMAL for files picked for compaction (which is the default value of access_hint_on_compaction_start).
The removal of this Hint results in degradation since these files are already hinted with POSIX_FADV_RANDOM in TableCache::GetTableReader when the files are opened (controlled by flag advise_random_on_open, true by default).

RocksDB plan to deprecate the flag access_hint_on_compaction_start in release 9.0.

To fix the above issue, its been decided that files undergoing compaction will be hinted with POSIX_FADV_NORMAL.

@Yuval-Ariel Yuval-Ariel added bug Something isn't working performance compaction labels Dec 24, 2023
@Yuval-Ariel Yuval-Ariel self-assigned this Dec 24, 2023
@Yuval-Ariel
Copy link
Contributor Author

Performance is restored after reverting the code in SetupForCompaction (facebook/rocksdb#11658)
Where red is the rebased branch with revert patch and green in current main.
image

Yuval-Ariel added a commit that referenced this issue Dec 25, 2023
In facebook/rocksdb#11658, files are no longer controlled by the access_hint_on_compaction_start flag by removing the content of SetupForCompaction.
However, this leads to degradation in compaction read speed when combined with compaction_readahead_size since once a file is opened, in linux, it is hinted with POSIX_FADV_RANDOM.

See #787 for full details.

Add back the default hint to avoid degradation in this scenario.
@Yuval-Ariel Yuval-Ariel linked a pull request Dec 25, 2023 that will close this issue
Yuval-Ariel added a commit that referenced this issue Dec 31, 2023
In facebook/rocksdb#11658, files are no longer controlled by the access_hint_on_compaction_start flag by removing the content of SetupForCompaction.
However, this leads to degradation in compaction read speed when combined with compaction_readahead_size since once a file is opened, in linux, it is hinted with POSIX_FADV_RANDOM.

See #787 for full details.

Add back the default hint to avoid degradation in this scenario.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compaction performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant