speedb-io / speedb

A RocksDB compliant high performance scalable embedded key-value store
https://www.speedb.io/
Apache License 2.0
912 stars 71 forks source link

set purges as Pri::LOW instead of HIGH #34

Closed Yuval-Ariel closed 2 years ago

Yuval-Ariel commented 2 years ago

reason for the change:

purges are deletions of obsolete compaction files. by doing the cleanup of these files in the HIGH priority thread pool, the flushes are hindered. we’d like to avoid disturbing the flushes and instead do this cleaning job as a LOW priority.

other notes:

this change is also favored when using the SPDB-680 (delete a file in chunks) feature since with it, the deletion takes longer which would further stall the flushes.

also, consider changing priority here as well db/db_impl/db_impl_compaction_flush.cc:

void DBImpl::BGWorkPurge(void* db) {
  IOSTATS_SET_THREAD_POOL_ID(Env::Priority::HIGH);
  TEST_SYNC_POINT("DBImpl::BGWorkPurge:start");
  reinterpret_cast<DBImpl*>(db)->BackgroundCallPurge();
  TEST_SYNC_POINT("DBImpl::BGWorkPurge:end");
}
assaf-speedb commented 2 years ago

Unit tests passed.

isaac-io commented 2 years ago

@erez-speedb this has been accidentally merged to main without running the performance tests. Please run them on commit d3a10864f41bec34f412b8203821d86aca366921 in main and compare the before and after with avoid_unnecessary_blocking_io after #183 is merged (I'll ping you when that happens).

isaac-io commented 2 years ago

183 has been merged. @erez-speedb, please run the aforementioned test on and before commit d3a10864f41bec34f412b8203821d86aca366921 as specified in my previous message.

Yuval-Ariel commented 2 years ago

@isaac-io we should also change the description here right?

include/rocksdb/options.h // If true, when PurgeObsoleteFile is called in CleanupIteratorState, we // schedule a background job in the flush job queue and delete obsolete files // in background. // Default: false bool background_purge_on_iterator_cleanup;

isaac-io commented 2 years ago

Yes. Would you be able to open a PR to fix this? I don't think that this requires an accompanying issue.