Skip to content

Commit

Permalink
set priority on TaskController instead of on postTask/yield (#27295)
Browse files Browse the repository at this point in the history
## Summary

passing both a signal and a priority to `postTask`/`yield` in chrome
causes memory to spike and potentially causes OOMs. a fix for this has
landed in chrome 118, but we can avoid the issue in earlier versions by
setting priority on just the TaskController instead.

https://bugs.chromium.org/p/chromium/issues/detail?id=1469367

## How did you test this change?
```
yarn test SchedulerPostTask
```
  • Loading branch information
noahlemen authored Aug 29, 2023
1 parent 2f36872 commit 4129ea8
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 9 deletions.
10 changes: 6 additions & 4 deletions packages/scheduler/src/__tests__/SchedulerPostTask-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ describe('SchedulerPostTask', () => {
const scheduler = {};
global.scheduler = scheduler;

scheduler.postTask = function (callback, {priority, signal}) {
scheduler.postTask = function (callback, {signal}) {
const {priority} = signal;
const id = idCounter++;
log(
`Post Task ${id} [${priority === undefined ? '<default>' : priority}]`,
Expand All @@ -94,7 +95,8 @@ describe('SchedulerPostTask', () => {
});
};

scheduler.yield = function ({priority, signal}) {
scheduler.yield = function ({signal}) {
const {priority} = signal;
const id = idCounter++;
log(`Yield ${id} [${priority === undefined ? '<default>' : priority}]`);
const controller = signal._controller;
Expand All @@ -111,8 +113,8 @@ describe('SchedulerPostTask', () => {
};

global.TaskController = class TaskController {
constructor() {
this.signal = {_controller: this};
constructor({priority}) {
this.signal = {_controller: this, priority};
}
abort() {
const task = taskQueue.get(this);
Expand Down
10 changes: 5 additions & 5 deletions packages/scheduler/src/forks/SchedulerPostTask.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import type {PriorityLevel} from '../SchedulerPriorities';

declare class TaskController {
constructor(priority?: string): TaskController;
constructor(options?: {priority?: string}): TaskController;
signal: mixed;
abort(): void;
}
Expand Down Expand Up @@ -95,9 +95,8 @@ export function unstable_scheduleCallback<T>(
break;
}

const controller = new TaskController();
const controller = new TaskController({priority: postTaskPriority});
const postTaskOptions = {
priority: postTaskPriority,
delay: typeof options === 'object' && options !== null ? options.delay : 0,
signal: controller.signal,
};
Expand Down Expand Up @@ -130,9 +129,10 @@ function runTask<T>(
if (typeof result === 'function') {
// Assume this is a continuation
const continuation: SchedulerCallback<T> = (result: any);
const continuationController = new TaskController();
const continuationOptions = {
const continuationController = new TaskController({
priority: postTaskPriority,
});
const continuationOptions = {
signal: continuationController.signal,
};
// Update the original callback node's controller, since even though we're
Expand Down

0 comments on commit 4129ea8

Please sign in to comment.