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.

Sign up for GitHub

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

Jump to bottom

Accurate memory tracking of immutable memory #132

Merged
merged 1 commit into from
Sep 28, 2022
Merged

Accurate memory tracking of immutable memory #132

merged 1 commit into from
Sep 28, 2022

Conversation

udi-speedb
Copy link
Contributor

No description provided.

@udi-speedb udi-speedb requested a review from isaac-io August 23, 2022 09:46
isaac-io
isaac-io suggested changes Aug 23, 2022
View reviewed changes
memtable/alloc_tracker.cc Outdated Show resolved Hide resolved
include/rocksdb/write_buffer_manager.h Outdated Show resolved Hide resolved
memtable/write_buffer_manager.cc Outdated Show resolved Hide resolved
memtable/write_buffer_manager.cc Outdated Show resolved Hide resolved
memtable/write_buffer_manager.cc Outdated Show resolved Hide resolved
memtable/write_buffer_manager.cc Outdated Show resolved Hide resolved
memtable/write_buffer_manager.cc Outdated Show resolved Hide resolved
memtable/write_buffer_manager.cc Outdated Show resolved Hide resolved
memtable/write_buffer_manager.cc Outdated Show resolved Hide resolved
isaac-io
isaac-io reviewed Aug 24, 2022
View reviewed changes
Copy link
Contributor

@isaac-io isaac-io left a comment

Choose a reason for hiding this comment

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

I left a few minor comments, but other than that LGTM. Can you also add a test or two to ensure the new functionality doesn't regress in the future?

include/rocksdb/write_buffer_manager.h Outdated Show resolved Hide resolved
include/rocksdb/write_buffer_manager.h
Comment on lines +96 to +100
// Note that the memory_inactive_ and memory_being_freed_ counters
// are NOT maintained when the WBM is disabled. In addition, memory_used_ is
// maintained only when enabled or cache is provided. Therefore, if switching
// from disabled to enabled, these counters will (or may) be invalid or may
// wraparound
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an upstream bug, regardless of the changes in this PR. Can we open a separate issue to fix this (i.e. track memory state even when disabled in order to avoid this scenario) and make this PR depend on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have opened https://github.com/speedb-io/speedb/issues/138
How do we make a PR depend on it and what does it mean practically?

Copy link
Contributor

@isaac-io isaac-io Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks!
We currently have no means of specifying that a PR depends on an issue or another PR because the automation is still a WIP, so generally making something dependent on another is currently simply adding Depends on ... to the PR description and avoiding merging it until the other one is resolved. My thinking was that since that's an upstream bug, it would be better if we fixed it before making the changes in this PR, so that it'd be easier to upstream. However, thinking about it some more, I don't think that the failure case justifies holding back this PR if you won't have time to open a PR for the other issue and add a regression test.

memtable/alloc_tracker.cc Outdated Show resolved Hide resolved
isaac-io
isaac-io reviewed Aug 24, 2022
View reviewed changes
memtable/write_buffer_manager.cc Outdated Show resolved Hide resolved
memtable/alloc_tracker.cc Outdated Show resolved Hide resolved
memtable/alloc_tracker.cc Outdated Show resolved Hide resolved
@udi-speedb udi-speedb force-pushed the 113-accurate-immutable-mem-tracking branch 2 times, most recently from bc26987 to 8118efb Compare August 25, 2022 03:19
isaac-io
isaac-io reviewed Aug 25, 2022
View reviewed changes
db/memtable_list.cc Outdated Show resolved Hide resolved
db/memtable_list.cc Outdated Show resolved Hide resolved
db/memtable_list_test.cc Outdated Show resolved Hide resolved
db/memtable_list_test.cc Outdated Show resolved Hide resolved
db/memtable_list_test.cc Outdated Show resolved Hide resolved
@udi-speedb udi-speedb force-pushed the 113-accurate-immutable-mem-tracking branch from a440fea to 408870c Compare August 25, 2022 10:29
isaac-io
isaac-io reviewed Aug 25, 2022
View reviewed changes
Copy link
Contributor

@isaac-io isaac-io left a comment

Choose a reason for hiding this comment

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

Sorry for the small bursts of comments. Only 2 minor comments this time, and hopefully it'll be the last round.

db/memtable_list.cc Outdated Show resolved Hide resolved
db/memtable.h Outdated Show resolved Hide resolved
db/memtable.h

uint64_t GetFileNumber() const { return file_number_; }

void SetFileNumber(uint64_t file_num) { file_number_ = file_num; }

void SetFlushInProgress(bool in_progress) {
if (in_progress && (flush_in_progress_ == false)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we simplify the check by wrapping this in if (in_progress != flush_in_progress_), so that inside we'll only need to check if the new value is true or not? It took me a couple of reads to understand that this is basically what the checks are all about.

@isaac-io isaac-io linked an issue Aug 30, 2022 that may be closed by this pull request
WriteBufferManager: accurate memory tracking of immutable memory #113
Closed
@udi-speedb udi-speedb force-pushed the 113-accurate-immutable-mem-tracking branch from 408870c to 6cc4a50 Compare September 5, 2022 12:10
@udi-speedb
Copy link
Contributor Author

@isaac-io - Please review the last commit. Thanks

@udi-speedb udi-speedb force-pushed the 113-accurate-immutable-mem-tracking branch from 6cc4a50 to 0aed128 Compare September 6, 2022 09:22
isaac-io
isaac-io approved these changes Sep 6, 2022
View reviewed changes
Copy link
Contributor

@isaac-io isaac-io left a comment

Choose a reason for hiding this comment

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

LGTM.

@udi-speedb udi-speedb force-pushed the 113-accurate-immutable-mem-tracking branch from 0aed128 to 6f2669a Compare September 6, 2022 11:19
@udi-speedb
Copy link
Contributor Author

I have squashed and pushed the branch

@isaac-io isaac-io force-pushed the 113-accurate-immutable-mem-tracking branch from 6f2669a to 7433613 Compare September 28, 2022 11:34
@isaac-io isaac-io merged commit 6810fd6 into main Sep 28, 2022
@isaac-io isaac-io deleted the 113-accurate-immutable-mem-tracking branch September 28, 2022 12:50
Yuval-Ariel pushed a commit that referenced this pull request Nov 23, 2022
@udi-speedb @Yuval-Ariel
Accurate memory tracking of immutable memory (#132)
8918b1b
Yuval-Ariel pushed a commit that referenced this pull request Nov 25, 2022
@udi-speedb @Yuval-Ariel
Accurate memory tracking of immutable memory (#132)
324514e
@Yuval-Ariel Yuval-Ariel mentioned this pull request Jan 11, 2023
Closed
Yuval-Ariel pushed a commit that referenced this pull request Apr 30, 2023
@udi-speedb @Yuval-Ariel
Accurate memory tracking of immutable memory (#132)
74ee2c0
udi-speedb added a commit that referenced this pull request May 3, 2023
@udi-speedb
Accurate memory tracking of immutable memory (#132) - adapted
2341d4b
Yuval-Ariel pushed a commit that referenced this pull request May 4, 2023
@udi-speedb @Yuval-Ariel
Accurate memory tracking of immutable memory (#132) - adapted
604cb54
udi-speedb added a commit that referenced this pull request Oct 31, 2023
@udi-speedb
Accurate memory tracking of immutable memory (#132) - adapted
e7fbdef
udi-speedb added a commit that referenced this pull request Dec 3, 2023
@udi-speedb
Accurate memory tracking of immutable memory (#132) - adapted
6e8c536
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isaac-io isaac-io isaac-io approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

WriteBufferManager: accurate memory tracking of immutable memory
2 participants
@udi-speedb @isaac-io