Skip to content

Delayed eviction from LRU - #87

Merged
crusaderky merged 1 commit into
dask:mainfrom
crusaderky:async_buffer
Mar 22, 2023
Merged

crusaderky merged 1 commit into
dask:mainfrom
crusaderky:async_buffer

Conversation

@crusaderky

@crusaderky crusaderky commented Mar 15, 2023 •

Copy link
Copy Markdown
Collaborator

@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 thread zict/lru.py
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

@crusaderky crusaderky Mar 20, 2023 •

Copy link
Copy Markdown
Collaborator Author

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 thread zict/lru.py
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
Copy Markdown
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.

@crusaderky
crusaderky marked this pull request as ready for review March 21, 2023 00:50
@crusaderky crusaderky mentioned this pull request Mar 21, 2023

@milesgranger milesgranger left a comment

Copy link
Copy Markdown

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
Copy Markdown
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