Repository navigation
Cap auto append-pool size and flush bulk workers in parallel at drain - #17
Open
adubovikov wants to merge 1 commit into
Open
adubovikov wants to merge 1 commit into
adubovikov wants to merge 1 commit into
Conversation
Two ingest regressions since a23ee39, measured with a 2M-row SIP INSERT on a 22-core box (DuckDB v1.5.6): - f8f20a0 removed AUTO_POOL_THREAD_CAP, so the auto pool grew to hardware_concurrency()/2 (11 workers here). Above ~4 optimistic writers the import leaves free blocks CHECKPOINT never reclaims: 690-770 MB file for 400 MB of data. Restore the cap of 4 for auto sizing and never exceed DuckDB's own threads setting (explicit rawduck_pool_threads still goes up to MAX_POOL_THREADS). Pipeline threads also respect threads now. - afb7532 made the drain keep every worker's collection uncompressed until commit, which serializes compression: 4.6 s -> 9.8 s. Workers holding at least one full row group are written in parallel again; smaller ones still stay in memory for the single CHECKPOINT pack, so small/medium tables keep the compact layout afb7532 was after. Result: 3.9 s / 403 MB / 0 free blocks (HEAD: 9.8 s / 690-760 MB), OTel traces/logs/metrics sizes unchanged, identical table content, all sqllogictests pass (663 assertions). 👹 Fixed by Goblin Co-Authored-By: Goblin <goblin@hep-labs.local>
This branch has not been deployed
This file contains hidden or 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
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.
Requested by Alex
Fixes #16.
Two ingest regressions on many-core machines since
a23ee39; details and bisect are in the issue.1. Cap the auto append-pool size (regression from #9).
PoolThreadCount()auto sizing is againmin(hw/2, AUTO_POOL_THREAD_CAP = 4), and it also never exceeds DuckDB'sthreadssetting (TaskScheduler::NumberOfThreads()).PipelineThreadCount()auto sizing also respectsthreads(it still scales tohw*2/3, up to 16).rawduck_pool_threadsstill goes up toMAX_POOL_THREADS.2. Write bulk workers in parallel at drain (regression from #11).
RawAppendPool::Drain, a worker holding at least one full row group (GetRowGroupSize()rows) is written by its own thread again, as before Feat/variant benchmark #11.rawduck_overlap_flushkeeps its mid-append meaning.flushedis avector<uint8_t>, notvector<bool>, because worker threads write it concurrently.Results (22 cores, DuckDB v1.5.6,
threads=8, defaultrawduck_*settings):78d3cd8raw_ingest_file* The spread on the medium tables is the pre-existing random free-block behaviour described in the issue. It shows up the same way with only change 1 applied and on
a23ee39. Used blocks are unchanged.count(*)plus per-columnsum(hash(col::VARCHAR))).make test_release: All tests passed (663 assertions, 2 skipped: quack, gRPC env).👹 Fixed by Goblin