Skip to content

Conversation

@ndr-ds
Copy link
Contributor

@ndr-ds ndr-ds commented Apr 8, 2025

Motivation

Previously the RocksDB settings were not dependant on the current hardware

Proposal

Make them dependent on the current hardware. These settings seem to work well even for the benchmarks, for example.

Test Plan

Ran benchmarks with these settings

Release Plan

  • Nothing to do / These changes follow the usual release cycle.

@ndr-ds ndr-ds force-pushed the 04-08-make_rocksdb_config_dependant_of_current_hardware branch from 205506b to b3ed57c Compare April 8, 2025 17:16
@ndr-ds ndr-ds force-pushed the 03-27-all_tasks_wait_before_starting branch from d9ef4ef to 538e084 Compare April 8, 2025 17:16
@ndr-ds ndr-ds requested review from Twey, afck, christos-h, jvff and ma2bd April 8, 2025 17:50
@ndr-ds ndr-ds marked this pull request as ready for review April 8, 2025 17:50
Comment on lines 320 to 328
options.set_level_zero_slowdown_writes_trigger(12);
options.set_level_zero_stop_writes_trigger(20);
options.increase_parallelism((num_cpus / 2).max(1));
options.set_max_background_jobs((num_cpus / 3).max(1));
Copy link
Contributor

@MathieuDutSik MathieuDutSik Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful to comment on the reason for choosing such parameters.

About commenting vs not commenting a good heuristic is to comment on things that
would take 10 minutes for a developer to find out. Here, I do not see why you chose
num_cpus / 2 and num_cpus / 3. That feels arbitrary and so deserves comment.

@ndr-ds ndr-ds force-pushed the 04-08-make_rocksdb_config_dependant_of_current_hardware branch from b3ed57c to cd7e8d6 Compare April 8, 2025 22:40
@ndr-ds ndr-ds force-pushed the 03-27-all_tasks_wait_before_starting branch from 538e084 to a8c7917 Compare April 8, 2025 22:40
@ndr-ds ndr-ds force-pushed the 04-08-make_rocksdb_config_dependant_of_current_hardware branch from cd7e8d6 to 212506f Compare April 9, 2025 20:45
@ndr-ds ndr-ds changed the base branch from 03-27-all_tasks_wait_before_starting to graphite-base/3740 April 10, 2025 19:49
@ndr-ds ndr-ds force-pushed the graphite-base/3740 branch from 344a842 to 4029c25 Compare April 10, 2025 19:56
@ndr-ds ndr-ds force-pushed the 04-08-make_rocksdb_config_dependant_of_current_hardware branch from 212506f to 936230c Compare April 10, 2025 19:56
@ndr-ds ndr-ds changed the base branch from graphite-base/3740 to 03-27-all_tasks_wait_before_starting April 10, 2025 19:56
@ndr-ds ndr-ds requested a review from MathieuDutSik April 10, 2025 19:59
Copy link
Contributor

@MathieuDutSik MathieuDutSik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ndr-ds ndr-ds force-pushed the 03-27-all_tasks_wait_before_starting branch from 4029c25 to 87a1499 Compare April 11, 2025 16:49
@ndr-ds ndr-ds force-pushed the 04-08-make_rocksdb_config_dependant_of_current_hardware branch from 936230c to 0840bd6 Compare April 11, 2025 16:49
This was referenced Apr 11, 2025
Copy link
Contributor Author

ndr-ds commented Apr 11, 2025

Merge activity

  • Apr 11, 4:39 PM EDT: A user started a stack merge that includes this pull request via Graphite.
  • Apr 11, 4:43 PM EDT: A user merged this pull request with Graphite.

@ndr-ds ndr-ds changed the base branch from 03-27-all_tasks_wait_before_starting to graphite-base/3740 April 11, 2025 20:40
@ndr-ds ndr-ds changed the base branch from graphite-base/3740 to main April 11, 2025 20:42
@ndr-ds ndr-ds force-pushed the 04-08-make_rocksdb_config_dependant_of_current_hardware branch from 0840bd6 to 5ca761a Compare April 11, 2025 20:42
@ndr-ds ndr-ds merged commit e759945 into main Apr 11, 2025
1 check passed
@ndr-ds ndr-ds deleted the 04-08-make_rocksdb_config_dependant_of_current_hardware branch April 11, 2025 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants