-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
add garbage collection #3094
add garbage collection #3094
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Ignored Deployments
|
🟢 CI successful 🟢Thanks |
Benchmark for 7306249Click to view benchmark
|
Benchmark for 701de04
Click to view full benchmark
|
Benchmark for d3506e2
Click to view full benchmark
|
Any chance you could add an explanation on how this works to |
Benchmark for f1790e1
Click to view full benchmark
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty complex change to an area of the code I'm not very familiar with.
let Cell::InitialValue{ content, .. } = replace(self, Cell::TrackedValueless { | ||
dependent_tasks, | ||
updates: 1, | ||
}) else { unreachable!() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also match std::mem::take(self) { ... }
(replacing it with a Cell:Empty
temporarily) and avoid this additional match. I think that's cleaner, and might even be compiled away.
let min_prio_that_needs_total_duration = if active { | ||
GcPriority::EmptyCells { | ||
total_compute_duration: to_exp_u8(last_duration.as_millis() as u64), | ||
age: Reverse(age), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerns of the Gc should not leak to this module, i.e. there should be a empty_cells(duration: Duration, age: Duration)
that creates this enum variant. It shouldn't have to deal with Reverse
and to_exp_u8
.
stats: &mut GcStats, | ||
backend: &MemoryBackend, | ||
turbo_tasks: &dyn TurboTasksBackendApi, | ||
) -> Option<GcPriority> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is huge. Can it be split into smaller, more digestible parts?
) | ||
#[allow(clippy::blocks_in_if_conditions)] | ||
while CURRENT_TASK_STATE | ||
.scope(Default::default(), async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this return a ControlFlow instead, just to make it clear that we're breaking or continuing a loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really can't track the code paths in manager.rs
, and I haven't even started task.rs
yet.
@@ -452,12 +511,16 @@ impl TaskScopeState { | |||
self.decrement_active_by(1, more_jobs); | |||
} | |||
/// decrement the active counter, returns list of child scopes that need to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Out of date comment?
event: Event, | ||
} | ||
|
||
impl PriorityPair { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs docs. It wasn't clear to me until I read memory_backend
that the high priority items use before/after hooks and low priority use callback styles.
} | ||
|
||
pub async fn run_low<T, F: Future<Output = T>>(&self, f: F) -> T { | ||
while self.current.load(Ordering::Acquire) != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: Why not just return a future that readies after the state is 0?
pub async fn run_low<T, F: Future<Output = T>>(&self, f: F) -> T { | ||
while self.current.load(Ordering::Acquire) != 0 { | ||
let listener = self.event.listen(); | ||
if self.current.load(Ordering::Acquire) != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: Can you explain why you double check?
if scope.state.lock().decrement_active_by(count, &mut queue) { | ||
self.gc_queue.task_might_become_inactive(task_id); | ||
} | ||
}); | ||
while let Some(scope) = queue.pop() { | ||
self.with_scope(scope, |scope| { | ||
scope.state.lock().decrement_active_by(count, &mut queue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the queued child tasks also become inactive?
remove updates tracking remove full cells
Benchmark for d63034e
Click to view full benchmark
|
Benchmark for 6ba7905
Click to view full benchmark
|
fixes WEB-265
fixes WEB-268
fixes WEB-4
fixes WEB-359