Skip to content

Commit 0ad6748

Browse files
wmertenswmertens
authored
Merge pull request #8143 from QwikDev/v2-skip-journal-during-diff
fix: skip apply journal during vnode diff
2 parents d5760ed + 07deb2c commit 0ad6748

File tree

3 files changed

+210
-12
lines changed

3 files changed

+210
-12
lines changed

‎packages/qwik/src/core/shared/scheduler-journal-block.unit.tsx‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
2+
import { $ } from '@qwik.dev/core';
3+
import { delay } from './utils/promises';
4+
import { Chore, createScheduler } from './scheduler';
5+
import { createDocument } from '@qwik.dev/core/testing';
6+
import { QContainerAttr } from './utils/markers';
7+
import { getDomContainer } from '../client/dom-container';
8+
import { ChoreArray } from '../client/chore-array';
9+
import { ChoreType } from './util-chore-type';
10+
import type { HostElement } from './types';
11+
import { _jsxSorted } from '../internal';
12+
import type { ElementVNode, VirtualVNode } from '../client/vnode-impl';
13+
import {
14+
vnode_insertBefore,
15+
vnode_locate,
16+
vnode_newUnMaterializedElement,
17+
vnode_newVirtual,
18+
} from '../client/vnode';
19+
20+
vi.mock('../client/vnode-diff', () => ({
21+
vnode_diff: vi.fn().mockImplementation(async () => {
22+
await delay(100);
23+
}),
24+
}));
25+
26+
describe('should block journal flush during node-diff and component runs', () => {
27+
let scheduler: ReturnType<typeof createScheduler> = null!;
28+
let document: ReturnType<typeof createDocument> = null!;
29+
let vBody: ElementVNode = null!;
30+
let vA: ElementVNode = null!;
31+
let vAHost: VirtualVNode = null!;
32+
let choreQueue: ChoreArray;
33+
let blockedChores: Set<Chore>;
34+
let runningChores: Set<Chore>;
35+
36+
async function waitForDrain() {
37+
await scheduler(ChoreType.WAIT_FOR_QUEUE).$returnValue$;
38+
}
39+
40+
beforeEach(() => {
41+
(globalThis as any).testLog = [];
42+
document = createDocument();
43+
document.body.setAttribute(QContainerAttr, 'paused');
44+
const container = getDomContainer(document.body);
45+
container.handleError = vi.fn();
46+
choreQueue = new ChoreArray();
47+
blockedChores = new Set();
48+
runningChores = new Set();
49+
scheduler = createScheduler(
50+
container,
51+
() => testLog.push('journalFlush'),
52+
choreQueue,
53+
blockedChores,
54+
runningChores
55+
);
56+
document.body.innerHTML = '<a :></a><b :></b>';
57+
vBody = vnode_newUnMaterializedElement(document.body);
58+
vA = vnode_locate(vBody, document.querySelector('a') as Element) as ElementVNode;
59+
vAHost = vnode_newVirtual();
60+
vAHost.setProp('q:id', 'A');
61+
vnode_insertBefore([], vA, vAHost, null);
62+
vi.useFakeTimers();
63+
});
64+
65+
afterEach(async () => {
66+
vi.useRealTimers();
67+
await waitForDrain();
68+
});
69+
70+
it('should block journal flush when NODE_DIFF is scheduled and executing', async () => {
71+
scheduler(
72+
ChoreType.NODE_DIFF,
73+
vAHost as HostElement,
74+
vAHost as HostElement,
75+
_jsxSorted('div', {}, null, null, 0, null)
76+
);
77+
78+
expect(choreQueue.length).toBe(1);
79+
vi.advanceTimersToNextTimer();
80+
expect(runningChores.size).toBe(1);
81+
await vi.advanceTimersByTimeAsync(20);
82+
// no journal flush even if time elapsed, because NODE_DIFF is running
83+
expect(testLog).toEqual([]);
84+
// finish VNODE_DIFF
85+
await vi.advanceTimersByTimeAsync(80);
86+
// no running chores
87+
expect(runningChores.size).toBe(0);
88+
// journal flush should have happened
89+
expect(testLog).toEqual(['journalFlush']);
90+
});
91+
92+
it('should block journal flush when NODE_DIFF and COMPONENT is scheduled and executing', async () => {
93+
scheduler(
94+
ChoreType.NODE_DIFF,
95+
vAHost as HostElement,
96+
vAHost as HostElement,
97+
_jsxSorted('div', {}, null, null, 0, null)
98+
);
99+
100+
expect(choreQueue.length).toBe(1);
101+
vi.advanceTimersToNextTimer();
102+
expect(runningChores.size).toBe(1);
103+
await vi.advanceTimersByTimeAsync(80);
104+
// no journal flush even if time elapsed, because NODE_DIFF is running
105+
expect(testLog).toEqual([]);
106+
// schedule component chore while NODE_DIFF is running
107+
scheduler(
108+
ChoreType.COMPONENT,
109+
vAHost as HostElement,
110+
$(async () => {
111+
await delay(50);
112+
}) as any,
113+
null
114+
);
115+
// finish VNODE_DIFF
116+
await vi.advanceTimersByTimeAsync(20);
117+
// no running chores
118+
expect(runningChores.size).toBe(1);
119+
// still no journal flush because COMPONENT is running
120+
expect(testLog).toEqual([]);
121+
await vi.advanceTimersByTimeAsync(20);
122+
// still no journal flush because COMPONENT is running
123+
expect(testLog).toEqual([]);
124+
// finish COMPONENT + next VNODE_DIFF
125+
await vi.advanceTimersByTimeAsync(110);
126+
expect(runningChores.size).toBe(0);
127+
expect(testLog).toEqual(['journalFlush']);
128+
});
129+
});

‎packages/qwik/src/core/shared/scheduler.ts‎

Lines changed: 80 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,8 @@ export const createScheduler = (
188188
let currentTime = performance.now();
189189
const nextTick = createNextTick(drainChoreQueue);
190190
let flushTimerId: number | null = null;
191+
let blockingChoresCount = 0;
192+
let currentChore: Chore | null = null;
191193

192194
function drainInNextTick() {
193195
if (!drainScheduled) {
@@ -269,6 +271,7 @@ export const createScheduler = (
269271
if (isTask) {
270272
(hostOrTask as Task).$flags$ |= TaskFlags.DIRTY;
271273
}
274+
272275
const chore: Chore<T> = {
273276
$type$: type,
274277
$idx$: isTask
@@ -339,6 +342,10 @@ This is often caused by modifying a signal in an already rendered component duri
339342
logWarn(warningMessage);
340343
DEBUG &&
341344
debugTrace('schedule.SKIPPED host is not updatable', chore, choreQueue, blockedChores);
345+
// Decrement counter if this was a blocking chore that we're skipping
346+
if (isRenderBlocking(type)) {
347+
blockingChoresCount--;
348+
}
342349
return chore;
343350
}
344351
}
@@ -367,8 +374,15 @@ This is often caused by modifying a signal in an already rendered component duri
367374
}
368375
}
369376

370-
addChore(chore, choreQueue);
371-
DEBUG && debugTrace('schedule', chore, choreQueue, blockedChores);
377+
addChoreAndIncrementBlockingCounter(chore, choreQueue);
378+
379+
DEBUG &&
380+
debugTrace(
381+
isRenderBlocking(type) ? `schedule (blocking ${blockingChoresCount})` : 'schedule',
382+
chore,
383+
choreQueue,
384+
blockedChores
385+
);
372386

373387
const runImmediately = (isServer && type === ChoreType.COMPONENT) || type === ChoreType.RUN_QRL;
374388

@@ -426,6 +440,16 @@ This is often caused by modifying a signal in an already rendered component duri
426440
}
427441

428442
function applyJournalFlush() {
443+
if (blockingChoresCount > 0) {
444+
DEBUG &&
445+
debugTrace(
446+
`journalFlush.BLOCKED (${blockingChoresCount} blocking chores)`,
447+
null,
448+
choreQueue,
449+
blockedChores
450+
);
451+
return;
452+
}
429453
if (!isJournalFlushRunning) {
430454
// prevent multiple journal flushes from running at the same time
431455
isJournalFlushRunning = true;
@@ -503,7 +527,7 @@ This is often caused by modifying a signal in an already rendered component duri
503527
if (vnode_isVNode(blockedChore.$host$)) {
504528
blockedChore.$host$.blockedChores?.delete(blockedChore);
505529
}
506-
addChore(blockedChore, choreQueue);
530+
addChoreAndIncrementBlockingCounter(blockedChore, choreQueue);
507531
DEBUG && debugTrace('schedule.UNBLOCKED', blockedChore, choreQueue, blockedChores);
508532
blockedChoresScheduled = true;
509533
}
@@ -515,13 +539,12 @@ This is often caused by modifying a signal in an already rendered component duri
515539
}
516540
};
517541

518-
let currentChore: Chore | null = null;
519-
520542
try {
521543
while (choreQueue.length) {
522544
currentTime = performance.now();
523545
const chore = (currentChore = choreQueue.shift()!);
524546
if (chore.$state$ !== ChoreState.NONE) {
547+
// Chore was already processed, counter already decremented in finishChore/handleError
525548
continue;
526549
}
527550

@@ -535,6 +558,10 @@ This is often caused by modifying a signal in an already rendered component duri
535558
if (vnode_isVNode(chore.$host$)) {
536559
chore.$host$.chores?.delete(chore);
537560
}
561+
// Decrement counter if this was a blocking chore that we're skipping
562+
if (isRenderBlocking(chore.$type$)) {
563+
blockingChoresCount--;
564+
}
538565
continue;
539566
}
540567

@@ -612,13 +639,40 @@ This is often caused by modifying a signal in an already rendered component duri
612639
if (vnode_isVNode(chore.$host$)) {
613640
chore.$host$.chores?.delete(chore);
614641
}
615-
DEBUG && debugTrace('execute.DONE', chore, choreQueue, blockedChores);
642+
643+
// Decrement blocking counter if this chore was blocking journal flush
644+
if (isRenderBlocking(chore.$type$)) {
645+
blockingChoresCount--;
646+
DEBUG &&
647+
debugTrace(
648+
`execute.DONE (blocking ${blockingChoresCount})`,
649+
chore,
650+
choreQueue,
651+
blockedChores
652+
);
653+
} else {
654+
DEBUG && debugTrace('execute.DONE', chore, choreQueue, blockedChores);
655+
}
616656
}
617657

618658
function handleError(chore: Chore, e: any) {
619659
chore.$endTime$ = performance.now();
620660
chore.$state$ = ChoreState.FAILED;
621-
DEBUG && debugTrace('execute.ERROR', chore, choreQueue, blockedChores);
661+
662+
// Decrement blocking counter if this chore was blocking journal flush
663+
if (isRenderBlocking(chore.$type$)) {
664+
blockingChoresCount--;
665+
DEBUG &&
666+
debugTrace(
667+
`execute.ERROR (blocking ${blockingChoresCount})`,
668+
chore,
669+
choreQueue,
670+
blockedChores
671+
);
672+
} else {
673+
DEBUG && debugTrace('execute.ERROR', chore, choreQueue, blockedChores);
674+
}
675+
622676
// If we used the result as promise, this won't exist
623677
chore.$reject$?.(e);
624678
container.handleError(e, chore.$host$);
@@ -808,8 +862,25 @@ This is often caused by modifying a signal in an already rendered component duri
808862
}
809863
return null;
810864
}
865+
866+
function addChoreAndIncrementBlockingCounter(chore: Chore, choreArray: ChoreArray) {
867+
if (addChore(chore, choreArray)) {
868+
blockingChoresCount++;
869+
}
870+
}
811871
};
812872

873+
export function addChore(chore: Chore, choreArray: ChoreArray): boolean {
874+
const idx = choreArray.add(chore);
875+
if (idx < 0) {
876+
if (vnode_isVNode(chore.$host$)) {
877+
(chore.$host$.chores ||= new ChoreArray()).add(chore);
878+
}
879+
return isRenderBlocking(chore.$type$);
880+
}
881+
return false;
882+
}
883+
813884
function vNodeAlreadyDeleted(chore: Chore): boolean {
814885
return !!(chore.$host$ && vnode_isVNode(chore.$host$) && chore.$host$.flags & VNodeFlags.Deleted);
815886
}
@@ -833,11 +904,8 @@ export function addBlockedChore(
833904
}
834905
}
835906

836-
export function addChore(chore: Chore, choreArray: ChoreArray) {
837-
const idx = choreArray.add(chore);
838-
if (idx < 0 && vnode_isVNode(chore.$host$)) {
839-
(chore.$host$.chores ||= new ChoreArray()).add(chore);
840-
}
907+
function isRenderBlocking(type: ChoreType): boolean {
908+
return type === ChoreType.NODE_DIFF || type === ChoreType.COMPONENT;
841909
}
842910

843911
function choreTypeToName(type: ChoreType): string {

‎packages/qwik/src/core/shared/scheduler.unit.tsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1216,6 +1216,7 @@ describe('scheduler', () => {
12161216
expect(chore.$type$).not.toBe(ChoreType.QRL_RESOLVE);
12171217
}
12181218

1219+
await waitForDrain();
12191220
nextTickSpy.mockRestore();
12201221
});
12211222
});

0 commit comments

Comments
 (0)