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

[CI ONLY] 9.7.4 candidate #13110

Merged
merged 3 commits into from
Nov 1, 2024
Merged

Commits on Nov 1, 2024

  1. Add a temporary hook for custom yielding in long-running op (facebook…

    …#13103)
    
    Summary:
    This is a simplified version of facebook#13096, which called for a way to hook into long-running loops completely within RocksDB to change their thread priority (or similar). The current prime hook point is `DBIter::FindNextUserEntryInternal` likely because of iterating over tombstones.
    
    This is implemented using the weak symbol hack for ease of back-porting/patching, and while we get to know potential future requirements better for integration into the public API. (Consider potential relationships to `Env::GetThreadStatusUpdater()` and `TransactionDBMutexFactory`.)
    
    Pull Request resolved: facebook#13103
    
    Test Plan:
    Performance validated with db_bench and DEBUG_LEVEL=0: `./db_bench --benchmarks=fillseq,deleterandom,readseq[-X100] --value_size=1 --num=1000000`
    
    No consistent difference seen; variances likely in how DB / executable / memory were laid out.
    
    ```
    With an empty hook:
    readseq [AVG    100 runs] : 1753018 (± 8850) ops/sec;   28.4 (± 0.1) MB/sec
    readseq [MEDIAN 100 runs] : 1763746 ops/sec;   28.6 MB/sec
    (recompile)
    readseq [AVG    100 runs] : 1789019 (± 10260) ops/sec;   29.0 (± 0.2) MB/sec
    readseq [MEDIAN 100 runs] : 1801849 ops/sec;   29.2 MB/sec
    
    Base:
    readseq [AVG    100 runs] : 1772196 (± 8240) ops/sec;   28.7 (± 0.1) MB/sec
    readseq [MEDIAN 100 runs] : 1780453 ops/sec;   28.9 MB/sec
    (recompile)
    readseq [AVG    100 runs] : 1777637 (± 7613) ops/sec;   28.8 (± 0.1) MB/sec
    readseq [MEDIAN 100 runs] : 1786657 ops/sec;   29.0 MB/sec
    
    With a functional hook (count number of calls into it):
    readseq [AVG    100 runs] : 1796733 (± 8854) ops/sec;   29.1 (± 0.1) MB/sec
    readseq [MEDIAN 100 runs] : 1804690 ops/sec;   29.3 MB/sec
    RocksDbThreadYield: 126915800
    (recompile)
    readseq [AVG    100 runs] : 1775371 (± 10529) ops/sec;   28.8 (± 0.2) MB/sec
    readseq [MEDIAN 100 runs] : 1789046 ops/sec;   29.0 MB/sec
    RocksDbThreadYield: 126977000
    
    Base:
    readseq [AVG    100 runs] : 1773071 (± 10657) ops/sec;   28.7 (± 0.2) MB/sec
    readseq [MEDIAN 100 runs] : 1783414 ops/sec;   28.9 MB/sec
    (recompile)
    readseq [AVG    100 runs] : 1750852 (± 10184) ops/sec;   28.4 (± 0.2) MB/sec
    readseq [MEDIAN 100 runs] : 1763587 ops/sec;   28.6 MB/sec
    ```
    
    Reviewed By: george-reynya
    
    Differential Revision: D65235379
    
    Pulled By: pdillinger
    
    fbshipit-source-id: 7829e4cc25a56d4c1801b8adf9c7f7aa49ab7aca
    pdillinger committed Nov 1, 2024
    Configuration menu
    Copy the full SHA
    520436a View commit details
    Browse the repository at this point in the history
  2. Fix a leak of open Blob files (facebook#13106)

    Summary:
    An earlier change (facebook@b34cef5) removed apparently unused functionality where an obsolete blob file number is passed for removal from TableCache, which manages SST files. This was actually relying on broken/fragile abstractions wherein TableCache and BlobFileCache share the same Cache and using the TableCache interface to manipulate blob file caching. No unit test was actually checking for removal of obsolete blob files from the cache (which is somewhat tricky to check and a second order correctness requirement).
    
    Here we fix the leak and add a DEBUG+ASAN-only check in DB::Close() that no obsolete files are lingering in the table/blob file cache.
    
    Fixes facebook#13066
    
    Important follow-up (FIXME): The added check discovered some apparent cases of leaked (into table_cache) SST file readers that would stick around until DB::Close(). Need to enable that check, diagnose, and fix.
    
    Pull Request resolved: facebook#13106
    
    Test Plan:
    added a check that is called during DB::Close in ASAN builds (to minimize paying the cost in all unit tests). Without the fix, the check failed in at least these tests:
    
    ```
    db_blob_basic_test DBBlobBasicTest.DynamicallyWarmCacheDuringFlush
    db_blob_compaction_test DBBlobCompactionTest.CompactionReadaheadMerge
    db_blob_compaction_test DBBlobCompactionTest.MergeBlobWithBase
    db_blob_compaction_test DBBlobCompactionTest.CompactionDoNotFillCache
    db_blob_compaction_test DBBlobCompactionTest.SkipUntilFilter
    db_blob_compaction_test DBBlobCompactionTest.CompactionFilter
    db_blob_compaction_test DBBlobCompactionTest.CompactionReadaheadFilter
    db_blob_compaction_test DBBlobCompactionTest.CompactionReadaheadGarbageCollection
    ```
    
    Reviewed By: ltamasi
    
    Differential Revision: D65296123
    
    Pulled By: pdillinger
    
    fbshipit-source-id: 2276d76482beb2c75c9010bc1bec070bb23a24c0
    pdillinger committed Nov 1, 2024
    Configuration menu
    Copy the full SHA
    0d378ba View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    3c27a3d View commit details
    Browse the repository at this point in the history