Skip to content

Commit

Permalink
skip chores on deleted vnodes
Browse files Browse the repository at this point in the history
  • Loading branch information
@Varixo
Varixo committed Aug 27, 2024
1 parent f67de35 commit 9aab08e
Show file tree
Hide file tree
Showing 14 changed files with 82 additions and 81 deletions.
10 changes: 6 additions & 4 deletions packages/qwik/src/core/api.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2041,6 +2041,8 @@ export type _VNode = _ElementVNode | _TextVNode | _VirtualVNode;

// @internal
export const enum _VNodeFlags {
// (undocumented)
Deleted = 32,
// (undocumented)
Element = 1,
// (undocumented)
Expand All @@ -2052,15 +2054,15 @@ export const enum _VNodeFlags {
// (undocumented)
INFLATED_TYPE_MASK = 15,
// (undocumented)
NAMESPACE_MASK = 96,
NAMESPACE_MASK = 192,
// (undocumented)
NEGATED_NAMESPACE_MASK = -97,
NEGATED_NAMESPACE_MASK = -193,
// (undocumented)
NS_html = 0,
// (undocumented)
NS_math = 64,
NS_math = 128,
// (undocumented)
NS_svg = 32,
NS_svg = 64,
// (undocumented)
Resolved = 16,
// (undocumented)
Expand Down
2 changes: 1 addition & 1 deletion packages/qwik/src/core/render/dom/notify-render.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ export const _hW = () => {
const [task] = useLexicalScope<[SubscriberEffect]>();
const container = getDomContainer(task.$el$ as Element);
const type = task.$flags$ & TaskFlags.VISIBLE_TASK ? ChoreType.VISIBLE : ChoreType.TASK;
container.$scheduler$.schedule(type, task as Task);
container.$scheduler$(type, task as Task);
};

const renderMarked = async (containerState: ContainerState): Promise<void> => {
Expand Down
12 changes: 6 additions & 6 deletions packages/qwik/src/core/state/common.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ export const isConnected = (sub: SubscriberEffect | SubscriberHost): boolean =>

/** @public */
export const unwrapProxy = <T>(proxy: T): T => {
return isObject(proxy) ? getProxyTarget<any>(proxy) ?? proxy : proxy;
return isObject(proxy) ? (getProxyTarget<any>(proxy) ?? proxy) : proxy;
};

export const getProxyTarget = <T extends object>(obj: T): T | undefined => {
Expand Down Expand Up @@ -496,10 +496,10 @@ export class LocalSubscriptionManager {
if (isComputedTask(host)) {
// scheduler(ChoreType.COMPUTED, host);
} else if (isResourceTask(host)) {
scheduler.schedule(ChoreType.RESOURCE, host);
scheduler(ChoreType.RESOURCE, host);
} else {
const task = host as Task;
scheduler.schedule(
scheduler(
task.$flags$ & TaskFlags.VISIBLE_TASK ? ChoreType.VISIBLE : ChoreType.TASK,
task
);
Expand All @@ -514,7 +514,7 @@ export class LocalSubscriptionManager {
host as fixMeAny,
ELEMENT_PROPS
);
scheduler.schedule(ChoreType.COMPONENT, host as fixMeAny, componentQrl, componentProps);
scheduler(ChoreType.COMPONENT, host as fixMeAny, componentQrl, componentProps);
}
} else {
const signal = sub[SubscriptionProp.SIGNAL];
Expand Down Expand Up @@ -563,7 +563,7 @@ export class LocalSubscriptionManager {
type == SubscriptionType.PROP_IMMUTABLE
);
} else {
scheduler.schedule(
scheduler(
ChoreType.NODE_DIFF,
host as fixMeAny,
sub[SubscriptionProp.ELEMENT] as fixMeAny,
Expand Down Expand Up @@ -598,7 +598,7 @@ function updateNodeProp(
const element = target[ElementVNodeProps.element] as Element;
container.$journal$.push(VNodeJournalOpCode.SetAttribute, element, propKey, value);
}
container.$scheduler$.schedule(ChoreType.JOURNAL_FLUSH);
container.$scheduler$(ChoreType.JOURNAL_FLUSH);
}

let __lastSubscription: Subscriptions | undefined;
Expand Down
2 changes: 1 addition & 1 deletion packages/qwik/src/core/use/use-task.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ export const useVisibleTaskQrl = (qrl: QRL<TaskFn>, opts?: OnVisibleTaskOptions)
useRunTask(task, eagerness);
if (!isServerPlatform()) {
qrl.$resolveLazy$(iCtx.$hostElement$ as fixMeAny);
iCtx.$container2$.$scheduler$.schedule(ChoreType.VISIBLE, task);
iCtx.$container2$.$scheduler$(ChoreType.VISIBLE, task);
}
} else {
const task = new Task(TaskFlags.VISIBLE_TASK, i, elCtx.$element$, qrl, undefined, null);
Expand Down
2 changes: 1 addition & 1 deletion packages/qwik/src/core/v2/client/dom-container.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ export class DomContainer extends _SharedContainer implements IClientContainer,
this.rendering = true;
this.renderDone = getPlatform().nextTick(() => {
// console.log('>>>> scheduleRender nextTick', !!this.rendering);
return maybeThen(this.$scheduler$.schedule(ChoreType.WAIT_FOR_ALL), () => {
return maybeThen(this.$scheduler$(ChoreType.WAIT_FOR_ALL), () => {
// console.log('>>>> scheduleRender done', !!this.rendering);
this.rendering = false;
});
Expand Down
4 changes: 2 additions & 2 deletions packages/qwik/src/core/v2/client/dom-render.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ export const render2 = async (
const container = getDomContainer(parent as HTMLElement) as DomContainer;
container.$serverData$ = opts.serverData!;
const host: HostElement = container.rootVNode as fixMeAny;
container.$scheduler$.schedule(ChoreType.NODE_DIFF, host, host, jsxNode as JSXNode);
await container.$scheduler$.schedule(ChoreType.WAIT_FOR_ALL);
container.$scheduler$(ChoreType.NODE_DIFF, host, host, jsxNode as JSXNode);
await container.$scheduler$(ChoreType.WAIT_FOR_ALL);
return {
cleanup: () => {
cleanup(container, container.rootVNode);
Expand Down
36 changes: 19 additions & 17 deletions packages/qwik/src/core/v2/client/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,29 +80,31 @@ export interface QNode extends Node {
* @internal
*/
export const enum VNodeFlags {
Element /* ****************** */ = 0b00_00001,
Virtual /* ****************** */ = 0b00_00010,
ELEMENT_OR_VIRTUAL_MASK /* ** */ = 0b00_00011,
ELEMENT_OR_TEXT_MASK /* ***** */ = 0b00_00101,
TYPE_MASK /* **************** */ = 0b00_00111,
INFLATED_TYPE_MASK /* ******* */ = 0b00_01111,
Text /* ********************* */ = 0b00_00100,
Element /* ****************** */ = 0b00_000001,
Virtual /* ****************** */ = 0b00_000010,
ELEMENT_OR_VIRTUAL_MASK /* ** */ = 0b00_000011,
ELEMENT_OR_TEXT_MASK /* ***** */ = 0b00_000101,
TYPE_MASK /* **************** */ = 0b00_000111,
INFLATED_TYPE_MASK /* ******* */ = 0b00_001111,
Text /* ********************* */ = 0b00_000100,
/// Extra flag which marks if a node needs to be inflated.
Inflated /* ***************** */ = 0b00_01000,
Inflated /* ***************** */ = 0b00_001000,
/// Marks if the `ensureProjectionResolved` has been called on the node.
Resolved /* ***************** */ = 0b00_10000,
Resolved /* ***************** */ = 0b00_010000,
/// Marks if the vnode is deleted.
Deleted /* ****************** */ = 0b00_100000,
/// Flags for Namespace
NAMESPACE_MASK /* *********** */ = 0b11_00000,
NEGATED_NAMESPACE_MASK /* ** */ = ~0b11_00000,
NS_html /* ****************** */ = 0b00_00000, // http://www.w3.org/1999/xhtml
NS_svg /* ******************* */ = 0b01_00000, // http://www.w3.org/2000/svg
NS_math /* ****************** */ = 0b10_00000, // http://www.w3.org/1998/Math/MathML
NAMESPACE_MASK /* *********** */ = 0b11_000000,
NEGATED_NAMESPACE_MASK /* ** */ = ~0b11_000000,
NS_html /* ****************** */ = 0b00_000000, // http://www.w3.org/1999/xhtml
NS_svg /* ******************* */ = 0b01_000000, // http://www.w3.org/2000/svg
NS_math /* ****************** */ = 0b10_000000, // http://www.w3.org/1998/Math/MathML
}

export const enum VNodeFlagsIndex {
mask /* ************* */ = ~0b11_11111,
negated_mask /* ****** */ = 0b11_11111,
shift /* ************* */ = 7,
mask /* ************* */ = ~0b11_111111,
negated_mask /* ****** */ = 0b11_111111,
shift /* ************* */ = 8,
}

export const enum VNodeProps {
Expand Down
22 changes: 8 additions & 14 deletions packages/qwik/src/core/v2/client/vnode-diff.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1019,7 +1019,7 @@ export const vnode_diff = (
const vNodeProps = vnode_getProp<any>(host, ELEMENT_PROPS, container.$getObjectById$);
shouldRender = shouldRender || propsDiffer(jsxProps, vNodeProps);
if (shouldRender) {
container.$scheduler$.schedule(ChoreType.COMPONENT, host, componentQRL, jsxProps);
container.$scheduler$(ChoreType.COMPONENT, host, componentQRL, jsxProps);
}
}
jsxValue.children != null && descendContentToProject(jsxValue.children, host);
Expand Down Expand Up @@ -1213,7 +1213,7 @@ export function cleanup(container: ClientContainer, vNode: VNode) {
if (type & VNodeFlags.Virtual) {
// Only virtual nodes have subscriptions
clearVNodeDependencies(vCursor);
cleanupVNodeChores(vNode, vParent, vCursor, container);
markVNodeAsDeleted(vNode, vParent, vCursor);
const seq = container.getHostProp<Array<any>>(vCursor as VirtualVNode, ELEMENT_SEQ);
if (seq) {
for (let i = 0; i < seq.length; i++) {
Expand All @@ -1222,7 +1222,7 @@ export function cleanup(container: ClientContainer, vNode: VNode) {
const task = obj;
clearSubscriberDependencies(task);
if (obj.$flags$ & TaskFlags.VISIBLE_TASK) {
container.$scheduler$.schedule(ChoreType.CLEANUP_VISIBLE, obj);
container.$scheduler$(ChoreType.CLEANUP_VISIBLE, obj);
} else {
cleanupTask(task);
}
Expand Down Expand Up @@ -1331,25 +1331,19 @@ function cleanupStaleUnclaimedProjection(journal: VNodeJournal, projection: VNod
}
}

function cleanupVNodeChores(
vNode: VNode,
vParent: VNode | null,
vCursor: VNode,
container: ClientContainer
) {
function markVNodeAsDeleted(vNode: VNode, vParent: VNode | null, vCursor: VNode) {
/**
* Cleanup stale chores for current vCursor, but only if it is not a projection. We need to do
* this to prevent stale chores from running after the vnode is removed. (for example signal
* subscriptions)
* Marks vCursor as deleted, but only if it is not a projection. We need to do this to prevent
* chores from running after the vnode is removed. (for example signal subscriptions)
*/
if (vNode !== vCursor) {
container.$scheduler$.cleanupStaleChores(vCursor as VirtualVNode);
vCursor[VNodeProps.flags] |= VNodeFlags.Deleted;
} else {
const currentVParent = vParent || vnode_getParent(vNode);
const isParentProjection =
currentVParent && vnode_getProp(currentVParent, QSlot, null) !== null;
if (!isParentProjection) {
container.$scheduler$.cleanupStaleChores(vCursor as VirtualVNode);
vCursor[VNodeProps.flags] |= VNodeFlags.Deleted;
}
}
}
Expand Down
29 changes: 19 additions & 10 deletions packages/qwik/src/core/v2/shared/scheduler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,13 @@ import { logWarn } from '../../util/log';
import { isPromise, maybeThen, maybeThenPassError, safeCall } from '../../util/promises';
import type { ValueOrPromise } from '../../util/types';
import { isDomContainer } from '../client/dom-container';
import { ElementVNodeProps, type ElementVNode, type VirtualVNode } from '../client/types';
import {
ElementVNodeProps,
VNodeFlags,
VNodeProps,
type ElementVNode,
type VirtualVNode,
} from '../client/types';
import {
vnode_documentPosition,
vnode_isVNode,
Expand Down Expand Up @@ -163,7 +169,7 @@ export const createScheduler = (
let currentChore: Chore | null = null;
let journalFlushScheduled: boolean = false;

return { schedule, cleanupStaleChores };
return schedule;

////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -283,6 +289,17 @@ export const createScheduler = (
// we have processed all of the chores up to the given chore.
break;
}
const isDeletedVNode =
nextChore.$host$ &&
vnode_isVNode(nextChore.$host$) &&
nextChore.$host$[VNodeProps.flags] & VNodeFlags.Deleted;
if (
isDeletedVNode &&
// we need to process cleanup tasks for deleted nodes
nextChore.$type$ !== ChoreType.CLEANUP_VISIBLE
) {
continue;
}
const returnValue = executeChore(nextChore);
if (isPromise(returnValue)) {
return returnValue.then(() => drainUpTo(runUptoChore));
Expand Down Expand Up @@ -380,14 +397,6 @@ export const createScheduler = (
return (chore.$returnValue$ = value);
});
}

function cleanupStaleChores(host: HostElement): void {
for (let i = choreQueue.length - 1; i >= 0; i--) {
if (choreQueue[i].$host$ === host) {
choreQueue.splice(i, 1);
}
}
}
};

const toNumber = (value: number | string): number => {
Expand Down
26 changes: 10 additions & 16 deletions packages/qwik/src/core/v2/shared/scheduler.unit.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,10 @@ describe('scheduler', () => {
});

it('should execute sort tasks', async () => {
scheduler.schedule(
ChoreType.TASK,
mockTask(vBHost1, { index: 2, qrl: $(() => testLog.push('b1.2')) })
);
scheduler.schedule(ChoreType.TASK, mockTask(vAHost, { qrl: $(() => testLog.push('a1')) }));
scheduler.schedule(ChoreType.TASK, mockTask(vBHost1, { qrl: $(() => testLog.push('b1.0')) }));
await scheduler.schedule(ChoreType.WAIT_FOR_ALL);
scheduler(ChoreType.TASK, mockTask(vBHost1, { index: 2, qrl: $(() => testLog.push('b1.2')) }));
scheduler(ChoreType.TASK, mockTask(vAHost, { qrl: $(() => testLog.push('a1')) }));
scheduler(ChoreType.TASK, mockTask(vBHost1, { qrl: $(() => testLog.push('b1.0')) }));
await scheduler(ChoreType.WAIT_FOR_ALL);
expect(testLog).toEqual([
'a1', // DepthFirst a host component is before b host component.
'b1.0', // Same component but smaller index.
Expand All @@ -69,23 +66,20 @@ describe('scheduler', () => {
]);
});
it('should execute visible tasks after journal flush', async () => {
scheduler.schedule(
scheduler(
ChoreType.TASK,
mockTask(vBHost2, { index: 2, qrl: $(() => testLog.push('b2.2: Task')) })
);
scheduler.schedule(
ChoreType.TASK,
mockTask(vBHost1, { qrl: $(() => testLog.push('b1.0: Task')) })
);
scheduler.schedule(
scheduler(ChoreType.TASK, mockTask(vBHost1, { qrl: $(() => testLog.push('b1.0: Task')) }));
scheduler(
ChoreType.VISIBLE,
mockTask(vBHost2, {
index: 2,
qrl: $(() => testLog.push('b2.2: VisibleTask')),
visible: true,
})
);
scheduler.schedule(
scheduler(
ChoreType.VISIBLE,
mockTask(vBHost1, {
qrl: $(() => {
Expand All @@ -94,13 +88,13 @@ describe('scheduler', () => {
visible: true,
})
);
scheduler.schedule(
scheduler(
ChoreType.COMPONENT,
vBHost1,
$(() => testLog.push('b1: Render')),
{}
);
await scheduler.schedule(ChoreType.WAIT_FOR_ALL);
await scheduler(ChoreType.WAIT_FOR_ALL);
expect(testLog).toEqual([
'b1.0: Task',
'b1: Render',
Expand Down
10 changes: 5 additions & 5 deletions packages/qwik/src/core/v2/signal/v2-signal.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,15 +301,15 @@ export const triggerEffects = (
} else if (effect.$flags$ & TaskFlags.RESOURCE) {
choreType = ChoreType.RESOURCE;
}
container.$scheduler$.schedule(choreType, effect);
container.$scheduler$(choreType, effect);
} else if (effect instanceof Signal2) {
// we don't schedule ComputedSignal/DerivedSignal directly, instead we invalidate it and
// and schedule the signals effects (recursively)
if (effect instanceof ComputedSignal2) {
// Ensure that the computed signal's QRL is resolved.
// If not resolved schedule it to be resolved.
if (!effect.$computeQrl$.resolved) {
container.$scheduler$.schedule(ChoreType.QRL_RESOLVE, null, effect.$computeQrl$);
container.$scheduler$(ChoreType.QRL_RESOLVE, null, effect.$computeQrl$);
}
}
(effect as ComputedSignal2<unknown> | DerivedSignal2<unknown>).$invalid$ = true;
Expand All @@ -327,11 +327,11 @@ export const triggerEffects = (
const qrl = container.getHostProp<QRL<(...args: any[]) => any>>(host, OnRenderProp);
assertDefined(qrl, 'Component must have QRL');
const props = container.getHostProp<any>(host, ELEMENT_PROPS);
container.$scheduler$.schedule(ChoreType.COMPONENT, host, qrl, props);
container.$scheduler$(ChoreType.COMPONENT, host, qrl, props);
} else if (property === EffectProperty.VNODE) {
const host: HostElement = effect as any;
const target = host;
container.$scheduler$.schedule(ChoreType.NODE_DIFF, host, target, signal as fixMeAny);
container.$scheduler$(ChoreType.NODE_DIFF, host, target, signal as fixMeAny);
} else {
const host: HostElement = effect as any;
const scopedStyleIdPrefix: string | null =
Expand All @@ -340,7 +340,7 @@ export const triggerEffects = (
value: signal,
scopedStyleIdPrefix,
};
container.$scheduler$.schedule(ChoreType.NODE_PROP, host, property, payload);
container.$scheduler$(ChoreType.NODE_PROP, host, property, payload);
}
};
effects.forEach(scheduleEffect);
Expand Down
Loading

0 comments on commit 9aab08e

Please sign in to comment.