Skip to content

Dirty memory: connect wbm to global delay - #423

Merged
Yuval-Ariel merged 6 commits into
mainfrom
dirty-mem-connect-wbm-to-global-delay
May 26, 2023
Merged

Yuval-Ariel merged 6 commits into
mainfrom
dirty-mem-connect-wbm-to-global-delay

Conversation

@Yuval-Ariel

Copy link
Copy Markdown
Contributor

closes #114

@Yuval-Ariel
Yuval-Ariel requested a review from udi-speedb March 12, 2023 12:32
@Yuval-Ariel Yuval-Ariel self-assigned this Mar 12, 2023
@Yuval-Ariel
Yuval-Ariel force-pushed the dirty-mem-connect-wbm-to-global-delay branch from 034ad23 to 1d36877 Compare March 12, 2023 12:48
@Yuval-Ariel
Yuval-Ariel force-pushed the dirty-mem-connect-wbm-to-global-delay branch from c6b29bc to 743a3d9 Compare March 19, 2023 12:45
@Yuval-Ariel

Copy link
Copy Markdown
Contributor Author

@udi-speedb , ready for review.
plz note that the first commit (92b1d8a ) is currently in another PR (#438) and will be merged before this PR so no need to review its contents.

@Yuval-Ariel
Yuval-Ariel force-pushed the dirty-mem-connect-wbm-to-global-delay branch from 743a3d9 to 2bfe7b9 Compare March 26, 2023 11:30
Comment thread db/write_controller.h Outdated
Comment thread db/write_controller.cc Outdated
Comment thread db/write_controller.cc Outdated
Comment thread db/write_controller.cc Outdated
Comment thread db/write_controller.cc Outdated
Comment thread db/write_controller.cc Outdated
Comment thread db/write_controller.cc Outdated
@udi-speedb

udi-speedb commented Mar 27, 2023 •

Copy link
Copy Markdown
Contributor

@Yuval-Ariel I have submitted an initial set of comments. I really think the interface of the write controller is needlessly complex and inelegant because it is defined in terms of a db id + cf id.
I suggest using uint64_t identifiers that the write controller will allocate. That way there is no need to worry about id-s wrapping around. You can just have a single uint64_t counter that will be incremented by 1 every allocation.
column families will register / de-register themselves from the write controller as well as the WBM, and the write controller will be unaware of the identity of its registered clients. It will also have a single unordered map to map client id-s (uint64_t) to their corresponding delays.

@Yuval-Ariel

Copy link
Copy Markdown
Contributor Author

i've addressed your concerns. plz rereview

@udi-speedb
udi-speedb self-requested a review March 30, 2023 04:24
Comment thread db/write_controller.h
Comment thread db/write_controller.cc
Comment thread db/write_controller.h
Comment thread db/write_controller.h
Comment thread db/write_controller.h
Comment thread db/column_family.cc Outdated
Comment thread db/write_controller.h Outdated
Comment thread db/column_family.cc
Comment thread db/column_family.cc
Comment thread db/column_family.cc Outdated
@udi-speedb

udi-speedb commented Apr 2, 2023 •

Copy link
Copy Markdown
Contributor

@Yuval-Ariel
I have submitted a first batch of comments as part of this PR.
In addition, I have pushed additional comments on files that are not part of this PR, but are related / part of the entire global delay / write controller feature(s).
These comments are added as a commit on the branch dirty-mem-connect-wbm-to-global-delay.old-code-comments
Please look at them as well and address them (you may do so as a new commit on that branch and / or we may discuss.

Please do not push a new commit to this PR / branch with updates yet.

@udi-speedb
udi-speedb self-requested a review April 2, 2023 15:05
@udi-speedb

Copy link
Copy Markdown
Contributor

Just thought of a potential issue to use pointers for client id-s of the WC. Since memory allocators may reuse released memory, you may have a race in which the memory of a deregistering client is released and immediately allocated to a new entity that registers with the WC. For example, a deleted CF and a newly created CF (could be deleted in one DB and created in another).

Comment thread include/rocksdb/write_buffer_manager.h Outdated
Comment thread include/rocksdb/write_buffer_manager.h Outdated
Comment thread memtable/write_buffer_manager.cc Outdated
Comment thread include/rocksdb/write_buffer_manager.h Outdated
Comment thread memtable/write_buffer_manager.cc
Comment thread memtable/write_buffer_manager.cc
Comment thread memtable/write_buffer_manager.cc
Comment thread memtable/write_buffer_manager.cc
Comment thread memtable/write_buffer_manager.cc
Comment thread memtable/write_buffer_manager.cc Outdated
@udi-speedb

Copy link
Copy Markdown
Contributor

@Yuval-Ariel Another batch of comment. Again, Please do not push a new commit yet. Thanks

@udi-speedb

Copy link
Copy Markdown
Contributor

I suggest adding a start_delay_percent flag to db_bench to be used when initializing the WBM.

Comment thread tools/db_crashtest.py Outdated
Comment thread tools/db_crashtest.py Outdated
Comment thread tools/db_crashtest.py Outdated
Comment thread memtable/write_buffer_manager.cc Outdated
Comment thread include/rocksdb/write_buffer_manager.h
Comment thread tools/db_bench_tool.cc Outdated
@udi-speedb

Copy link
Copy Markdown
Contributor

@Yuval-Ariel A few more minor comments. I am not done yet.
Please address comments and push a commit with updates following the comments I have given thus far. Thanks

@udi-speedb
udi-speedb self-requested a review April 9, 2023 12:59
@Yuval-Ariel

Copy link
Copy Markdown
Contributor Author

@udi-speedb , this is waiting for your review

@udi-speedb

Copy link
Copy Markdown
Contributor

@Yuval-Ariel - Indeed. Will resume the review soon, once the bulk of the log parser work is over.

Comment thread db/write_controller.h
Comment thread db/write_controller.h Outdated
Comment thread db/write_controller.cc
Comment thread db/write_controller.cc Outdated
Comment thread db/write_controller.h
Comment thread db/column_family.cc
Comment thread db/compaction/compaction_job.cc
Comment thread include/rocksdb/write_buffer_manager.h
Comment thread db/write_controller.cc Outdated
Comment thread db/write_controller.cc Outdated
@udi-speedb

Copy link
Copy Markdown
Contributor

@Yuval-Ariel Another batch of comments. I apologize but I still have code in this pr to review.
Please do not put another commit yet. Thanks

@udi-speedb

Copy link
Copy Markdown
Contributor

@Yuval-Ariel - Please push whatever updates you have in a new commit. Thanks

Comment thread db/global_write_controller_test.cc Outdated
Comment thread db/global_write_controller_test.cc
Comment thread db/global_write_controller_test.cc
Comment thread db/global_write_controller_test.cc
Comment thread db/global_write_controller_test.cc
Comment thread db/global_write_controller_test.cc
Comment thread db/write_controller.h
@udi-speedb

Copy link
Copy Markdown
Contributor

@yuval - I have submitted my last comments.

@erez-speedb

Copy link
Copy Markdown

@Yuval-Ariel perf tests were done on the #114 issue.
Whats the difference ?

@Yuval-Ariel

Copy link
Copy Markdown
Contributor Author

no difference. no need to rerun them

@Yuval-Ariel
Yuval-Ariel force-pushed the dirty-mem-connect-wbm-to-global-delay branch from ba6a3de to 998751b Compare May 23, 2023 13:09
@Yuval-Ariel

Copy link
Copy Markdown
Contributor Author

i've rebased to rsolve conflicts and added to the HISTORY. plz recheck @udi-speedb

@Yuval-Ariel
Yuval-Ariel requested a review from udi-speedb May 23, 2023 13:16
@Yuval-Ariel Yuval-Ariel linked an issue May 24, 2023 that may be closed by this pull request
@Yuval-Ariel

Yuval-Ariel commented May 24, 2023 •

Copy link
Copy Markdown
Contributor Author

merged main to resolve conflicts

@Yuval-Ariel
Yuval-Ariel merged commit 6ede30d into main May 26, 2023
@Yuval-Ariel
Yuval-Ariel deleted the dirty-mem-connect-wbm-to-global-delay branch May 26, 2023 08:31
Yuval-Ariel added a commit that referenced this pull request Jun 12, 2023
…549)

During #423 , the WriteBufferManager ctor parameter allow_stall was renamed to allow_delays_and_stalls to reflect its new capabilities (delaying in addition to stalling (stopping) writes). This creates confusion and unwanted behavior so this PR reverts this change and the new functionality of #423 is controller via the old WriteBufferManager ctor parameter named allow_stall.

This commit also sets back the allow_stall default value to false to avoid changing current expected behavior.
udi-speedb pushed a commit that referenced this pull request Nov 19, 2023
Allow slowing down writes based on memory consumption by having the WBM raise a delay requirement in the WriteController.
The delay requirement is stored in the WC as part of the Global Delay feature (#346). The same way a CF signals the WC that it has a delay requirement.
udi-speedb pushed a commit that referenced this pull request Nov 22, 2023
…549)

During #423 , the WriteBufferManager ctor parameter allow_stall was renamed to allow_delays_and_stalls to reflect its new capabilities (delaying in addition to stalling (stopping) writes). This creates confusion and unwanted behavior so this PR reverts this change and the new functionality of #423 is controller via the old WriteBufferManager ctor parameter named allow_stall.

This commit also sets back the allow_stall default value to false to avoid changing current expected behavior.
udi-speedb pushed a commit that referenced this pull request Dec 5, 2023
Allow slowing down writes based on memory consumption by having the WBM raise a delay requirement in the WriteController.
The delay requirement is stored in the WC as part of the Global Delay feature (#346). The same way a CF signals the WC that it has a delay requirement.
udi-speedb pushed a commit that referenced this pull request Dec 5, 2023
…549)

During #423 , the WriteBufferManager ctor parameter allow_stall was renamed to allow_delays_and_stalls to reflect its new capabilities (delaying in addition to stalling (stopping) writes). This creates confusion and unwanted behavior so this PR reverts this change and the new functionality of #423 is controller via the old WriteBufferManager ctor parameter named allow_stall.

This commit also sets back the allow_stall default value to false to avoid changing current expected behavior.
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.

Dirty Memory Manager WriteBufferManager: allow slowing down writes based on total memory usage

3 participants