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.

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

chore: ClearInternal now can clear partially #3867

Merged
merged 2 commits into from
Oct 4, 2024
Merged

chore: ClearInternal now can clear partially #3867

merged 2 commits into from
Oct 4, 2024

Conversation

romange
Copy link
Collaborator

@romange romange commented Oct 4, 2024

Intended for future use - to deallocate large objects gradually. Currently nothing is changed in the functionality besides some cleanups.

Intended for future use - to deallocate large objects gradually.
Currently nothing is changed in the functionality besides some cleanups.

Signed-off-by: Roman Gershman <[email protected]>
@romange romange requested a review from kostasrim October 4, 2024 15:34
constexpr unsigned kArrLen = 32;
ClearItem arr[kArrLen];
unsigned len = 0;

for (auto it = entries_.begin(); it != entries_.end(); ++it) {
if (it->IsEmpty())
size_t end = min<size_t>(entries_.size(), start + limit);
Copy link
Contributor

Choose a reason for hiding this comment

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

you mean limit - start here right ? Cause otherwise:

entries_.size()=10, start=3, limit=8 then end=10 and you will iterate from start=3 up to end=10 when you wanted to start from 3 and end up to limit=8.

Is there something I am missing here?

Copy link
Contributor

@kostasrim kostasrim Oct 4, 2024

Choose a reason for hiding this comment

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

unless you mean limit as count that is, how many elements from start you want to delete. (cause in my mind limit is more of like an index limit rather than the total elements you want to delete starting at pos start)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I meant count, not limit

Copy link
Contributor

Choose a reason for hiding this comment

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

uh gotcha

kostasrim
kostasrim previously approved these changes Oct 4, 2024
@kostasrim
Copy link
Contributor

Unit failure

2024-10-04T16:16:10.3912959Z 46: /__w/dragonfly/dragonfly/src/server/tiering/op_manager_test.cc:90: Failure
2024-10-04T16:16:10.3913699Z 46: Expected equality of these values:
2024-10-04T16:16:10.3914142Z 46:   GetStats().disk_stats.allocated_bytes
2024-10-04T16:16:10.3914564Z 46:     Which is: 413696
2024-10-04T16:16:10.3914885Z 46:   100 * kPageSize
2024-10-04T16:16:10.3915188Z 46:     Which is: 409600

We have an issue as well #3743

@romange romange merged commit 07e0b9d into main Oct 4, 2024
12 checks passed
@romange romange deleted the Pr3 branch October 4, 2024 19:50
kostasrim pushed a commit that referenced this pull request Oct 7, 2024
* chore: ClearInternal now can clear partially

Intended for future use - to deallocate large objects gradually.
Currently nothing is changed in the functionality besides some cleanups.
---------

Signed-off-by: Roman Gershman <[email protected]>
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.

2 participants