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

Flush when too many handles are locked #405

Merged
merged 3 commits into from
Jan 13, 2025
Merged

Conversation

nathanielsimard
Copy link
Member

@nathanielsimard nathanielsimard commented Jan 9, 2025

It helps when running wgpu tests or with autotune when tensors are allocated, but the kernel is never executed.

}

impl MemoryLock {
/// Create a new memory lock with the given flushing threshold.
pub fn new(flush_threshold: usize) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should rename the function to with_threshold?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, there is no other way to create it, you need a threshold to create the memory lock.

Comment on lines 26 to 30
pub fn should_flush(&self) -> bool {
// For now we only consider the number of handles locked, but we may consider the amount in
// bytes at some point.
self.locked.len() >= self.flush_threshold
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding its usage, I suggest renaming to has_reach_threshold. At least, when use in WgpuStream, we don't flush the memory lock. The current name suggests that after this function returns true, we should flush the memory lock.

@nathanielsimard nathanielsimard merged commit c63a62b into main Jan 13, 2025
5 checks passed
@nathanielsimard nathanielsimard deleted the fix/wgpu/memory-lock branch January 13, 2025 22:46
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