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

Delayed eviction from LRU #87

Merged
merged 1 commit into from
Mar 22, 2023
Merged

Delayed eviction from LRU #87

merged 1 commit into from
Mar 22, 2023

Conversation

crusaderky
Copy link
Collaborator

@crusaderky crusaderky commented Mar 15, 2023

@crusaderky crusaderky self-assigned this Mar 15, 2023
@crusaderky crusaderky force-pushed the async_buffer branch 7 times, most recently from 5685f97 to c34d681 Compare March 20, 2023 22:40
@crusaderky crusaderky changed the title WIP Asynchronous disk access Delayed eviction from LRU Mar 20, 2023
Comment on lines +101 to +104
if self.weights[key] > self.n and key not in self.heavy:
# weight(value) > n and evicting the key we just inserted failed.
# Evict the rest of the LRU instead.
try:
while len(self.d) > 1:
self.evict()
except Exception:
pass
Copy link
Collaborator Author

@crusaderky crusaderky Mar 20, 2023

Choose a reason for hiding this comment

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

The behaviour implemented by this blurb is very opinable. It's there mostly to retain functional compatibility with main (there are several tests in dask/distributed that rely on this behaviour too).
Ultimately, once we transition to async set, this will become dead code (at least in distributed).

Comment on lines +170 to +162
# If we are evicting a heavy key we just inserted and one of the callbacks
# fails, put it at the bottom of the LRU instead of the top. This way lighter
# keys will have a chance to be evicted first and make space.
self.heavy.discard(key)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Again, very opinable behaviour that is here chiefly to retain functional compatibility with main.

Copy link

@milesgranger milesgranger left a comment

Choose a reason for hiding this comment

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

From what I see (and understand of zict in general) this looks overall very good to me.

As a side note, I'd still advocate for another name other than calling these things 'thread-safe'. It doesn't seem to use "traditional" thread safety thru locks, atomics or being lock-free thread-safe via inter-thread communication thru queues/channels but rather silently ignoring expected exceptions from threading conflicts. From my current understanding, this doesn't constitute being thread-safe. It appears to be more 'obstruction free thread-able by suppressing expected exceptions; hope your underlying things are thread-safe'.

But I say this as a noob in zict so please disregard if this is not applicable. :)

@crusaderky
Copy link
Collaborator Author

'obstruction free thread-able by suppressing expected exceptions; hope your underlying things are thread-safe'.

Please check yourself that your underlying things are thread-safe, in the specific documented methods.

@crusaderky crusaderky merged commit 9d2718f into dask:main Mar 22, 2023
@crusaderky crusaderky deleted the async_buffer branch March 22, 2023 15:01
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
2 participants