Skip to content

Thread-safe Buffer.__items__; proper KeysView, ItemsView, ValuesView #93

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

Merged
merged 4 commits into from
Mar 29, 2023

Conversation

crusaderky
Copy link
Collaborator

@crusaderky crusaderky self-assigned this Mar 27, 2023
Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

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

I have two questions regarding the implementation and a few nits.

@@ -165,3 +178,25 @@ def flush(self) -> None:

def close(self) -> None:
close(self.fast, self.slow)


class BufferItemsView(ItemsView[KT, VT]):
Copy link
Member

Choose a reason for hiding this comment

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

Should we also override __contains__ in BufferItemsView to avoid changing the LRU?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason why __iter__ avoids changing the LRU is because it would raise RuntimeError halfway through. If you're testing if your buffer contains a specific key/value pair, and that causes the value to be unevicted from slow, then I don't see why it should bypass the principle of temporal locality.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation; now it makes sense to me. I had assumed we wanted this to be a general property of the views.

If you're testing if your buffer contains a specific key/value pair, and that causes the value to be unevicted from slow, then I don't see why it should bypass the principle of temporal locality.

Sounds good to me!

@crusaderky
Copy link
Collaborator Author

@hendrikmakait all review comments have been addressed

Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

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

Thanks, @crusaderky!

@crusaderky crusaderky merged commit 9f62731 into dask:main Mar 29, 2023
@crusaderky crusaderky deleted the iters branch March 29, 2023 12:05
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.

Asynchronous Disk Access in Workers Dictionary views
2 participants