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

[FEA] Remove reliance on handle lock to check spillability #11830

Open
abellina opened this issue Dec 6, 2024 · 0 comments
Open

[FEA] Remove reliance on handle lock to check spillability #11830

abellina opened this issue Dec 6, 2024 · 0 comments
Assignees
Labels
feature request New feature or request

Comments

@abellina
Copy link
Collaborator

abellina commented Dec 6, 2024

For simplicity #11747 locks the whole spillable handle while it is spilling. This is because it wants to populate the spilled handle atomically w.r.t. the user trying to materialize the buffer. Why? Because we don't want to return an object on the device that is about to be removed, and we also need to make sure all the host-side objects are created and initialized in a thread safe way.

That said, this same lock is used to figure out if a handle is spillable:

def spillable = synchronized { ... }
def spill() = { ... synchronized { ... } }
def materialize() { synchronized { ... } ... }

Therefore a second thread trying to spill (asking each handle if they are spillable) is going to be blocked by a first thread that is currently in the synchronized block of spill. This synchronized block is long-running as it is held during IO. materialize should be blocked on the synchronized and it's OK if it has to wait for the IO, in my opinion. We can think about what it means to relax materialize synchronization w.r.t. IO later.

The desired solution makes spillable not depend on the IO happening in spill while keeping the state consistency needed such that a user calling materialize is able to get consistent state.

@abellina abellina added ? - Needs Triage Need team to review and classify feature request New feature or request labels Dec 6, 2024
@mattahrens mattahrens removed the ? - Needs Triage Need team to review and classify label Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants