From 3394b38b52c248c05ba625012494173f7eda128d Mon Sep 17 00:00:00 2001 From: davidaghassi Date: Sat, 28 Sep 2024 14:53:23 -0700 Subject: [PATCH 1/4] fix: improve long running task performance in query core This addresses two hot spots we have noticed on a large scale enterprise app when profiling with chrome. These changes help to cut down on long running tasks when there are many query calls on the page --- packages/query-core/src/queriesObserver.ts | 69 ++++++++++------------ packages/query-core/src/queryObserver.ts | 28 ++++----- 2 files changed, 44 insertions(+), 53 deletions(-) diff --git a/packages/query-core/src/queriesObserver.ts b/packages/query-core/src/queriesObserver.ts index 2c58e9221c..6c6ca0f1c9 100644 --- a/packages/query-core/src/queriesObserver.ts +++ b/packages/query-core/src/queriesObserver.ts @@ -204,59 +204,50 @@ export class QueriesObserver< #findMatchingObservers( queries: Array, ): Array { - const prevObservers = this.#observers const prevObserversMap = new Map( - prevObservers.map((observer) => [observer.options.queryHash, observer]), + this.#observers.map((observer) => [observer.options.queryHash, observer]), ) - const defaultedQueryOptions = queries.map((options) => - this.#client.defaultQueryOptions(options), - ) - - const matchingObservers: Array = - defaultedQueryOptions.flatMap((defaultedOptions) => { - const match = prevObserversMap.get(defaultedOptions.queryHash) - if (match != null) { - return [{ defaultedQueryOptions: defaultedOptions, observer: match }] - } - return [] - }) - - const matchedQueryHashes = new Set( - matchingObservers.map((match) => match.defaultedQueryOptions.queryHash), - ) - const unmatchedQueries = defaultedQueryOptions.filter( - (defaultedOptions) => !matchedQueryHashes.has(defaultedOptions.queryHash), - ) + const matchingObservers: Array = [] + const unmatchedQueries: Array = [] - const getObserver = (options: QueryObserverOptions): QueryObserver => { + queries.forEach((options) => { const defaultedOptions = this.#client.defaultQueryOptions(options) - const currentObserver = this.#observers.find( - (o) => o.options.queryHash === defaultedOptions.queryHash, - ) - return ( - currentObserver ?? new QueryObserver(this.#client, defaultedOptions) - ) - } + const match = prevObserversMap.get(defaultedOptions.queryHash) + if (match) { + matchingObservers.push({ + defaultedQueryOptions: defaultedOptions, + observer: match, + }) + } else { + unmatchedQueries.push(defaultedOptions) + } + }) const newOrReusedObservers: Array = unmatchedQueries.map((options) => { + const existingObserver = this.#observers.find( + (o) => o.options.queryHash === options.queryHash, + ) return { defaultedQueryOptions: options, - observer: getObserver(options), + observer: + existingObserver ?? new QueryObserver(this.#client, options), } }) - const sortMatchesByOrderOfQueries = ( - a: QueryObserverMatch, - b: QueryObserverMatch, - ): number => - defaultedQueryOptions.indexOf(a.defaultedQueryOptions) - - defaultedQueryOptions.indexOf(b.defaultedQueryOptions) + const allObservers = matchingObservers.concat(newOrReusedObservers) - return matchingObservers - .concat(newOrReusedObservers) - .sort(sortMatchesByOrderOfQueries) + return allObservers.sort((a, b) => { + return ( + queries.findIndex( + (q) => q.queryHash === a.defaultedQueryOptions.queryHash, + ) - + queries.findIndex( + (q) => q.queryHash === b.defaultedQueryOptions.queryHash, + ) + ) + }) } #onUpdate(observer: QueryObserver, result: QueryObserverResult): void { diff --git a/packages/query-core/src/queryObserver.ts b/packages/query-core/src/queryObserver.ts index 2971bc6d1a..0e72ab2261 100644 --- a/packages/query-core/src/queryObserver.ts +++ b/packages/query-core/src/queryObserver.ts @@ -263,21 +263,21 @@ export class QueryObserver< result: QueryObserverResult, onPropTracked?: (key: keyof QueryObserverResult) => void, ): QueryObserverResult { - const trackedResult = {} as QueryObserverResult - - Object.keys(result).forEach((key) => { - Object.defineProperty(trackedResult, key, { - configurable: false, - enumerable: true, - get: () => { - this.trackProp(key as keyof QueryObserverResult) - onPropTracked?.(key as keyof QueryObserverResult) - return result[key as keyof QueryObserverResult] - }, - }) - }) + const handler: ProxyHandler> = { + get: (target, key: keyof QueryObserverResult) => { + if (key in target) { + this.trackProp(key) + if (onPropTracked) { + onPropTracked(key) + } + return target[key] + } + // Ensure all paths are covered by explicitly handling "undefined" properties + return undefined + }, + } - return trackedResult + return new Proxy(result, handler) } trackProp(key: keyof QueryObserverResult) { From c1d125c42738caac292e6d03035c675a4ea0418a Mon Sep 17 00:00:00 2001 From: davidaghassi Date: Tue, 1 Oct 2024 11:10:18 -0700 Subject: [PATCH 2/4] refactor: remove two more arrays to be more efficient --- packages/query-core/src/queriesObserver.ts | 29 ++++++++-------------- 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/packages/query-core/src/queriesObserver.ts b/packages/query-core/src/queriesObserver.ts index 6c6ca0f1c9..588df86f55 100644 --- a/packages/query-core/src/queriesObserver.ts +++ b/packages/query-core/src/queriesObserver.ts @@ -208,37 +208,30 @@ export class QueriesObserver< this.#observers.map((observer) => [observer.options.queryHash, observer]), ) - const matchingObservers: Array = [] - const unmatchedQueries: Array = [] + const observers: Array = [] queries.forEach((options) => { const defaultedOptions = this.#client.defaultQueryOptions(options) const match = prevObserversMap.get(defaultedOptions.queryHash) if (match) { - matchingObservers.push({ + observers.push({ defaultedQueryOptions: defaultedOptions, observer: match, }) } else { - unmatchedQueries.push(defaultedOptions) - } - }) - - const newOrReusedObservers: Array = - unmatchedQueries.map((options) => { const existingObserver = this.#observers.find( - (o) => o.options.queryHash === options.queryHash, + (o) => o.options.queryHash === defaultedOptions.queryHash, ) - return { - defaultedQueryOptions: options, + observers.push({ + defaultedQueryOptions: defaultedOptions, observer: - existingObserver ?? new QueryObserver(this.#client, options), - } - }) - - const allObservers = matchingObservers.concat(newOrReusedObservers) + existingObserver ?? + new QueryObserver(this.#client, defaultedOptions), + }) + } + }) - return allObservers.sort((a, b) => { + return observers.sort((a, b) => { return ( queries.findIndex( (q) => q.queryHash === a.defaultedQueryOptions.queryHash, From 73b683f6c68339a60bf88d798c0565cda24b2efe Mon Sep 17 00:00:00 2001 From: davidaghassi Date: Thu, 3 Oct 2024 10:12:51 -0700 Subject: [PATCH 3/4] refactor: remove queryObserver improvement from PR --- packages/query-core/src/queryObserver.ts | 95 +++++------------------- 1 file changed, 18 insertions(+), 77 deletions(-) diff --git a/packages/query-core/src/queryObserver.ts b/packages/query-core/src/queryObserver.ts index 62b458bfd0..2971bc6d1a 100644 --- a/packages/query-core/src/queryObserver.ts +++ b/packages/query-core/src/queryObserver.ts @@ -1,8 +1,3 @@ -import { focusManager } from './focusManager' -import { notifyManager } from './notifyManager' -import { fetchState } from './query' -import { Subscribable } from './subscribable' -import { pendingThenable } from './thenable' import { isServer, isValidTimeout, @@ -13,9 +8,12 @@ import { shallowEqualObjects, timeUntilStale, } from './utils' +import { notifyManager } from './notifyManager' +import { focusManager } from './focusManager' +import { Subscribable } from './subscribable' +import { fetchState } from './query' import type { FetchOptions, Query, QueryState } from './query' import type { QueryClient } from './queryClient' -import type { PendingThenable, Thenable } from './thenable' import type { DefaultError, DefaultedQueryObserverOptions, @@ -59,7 +57,6 @@ export class QueryObserver< TQueryData, TQueryKey > - #currentThenable: Thenable #selectError: TError | null #selectFn?: (data: TQueryData) => TData #selectResult?: TData @@ -85,13 +82,6 @@ export class QueryObserver< this.#client = client this.#selectError = null - this.#currentThenable = pendingThenable() - if (!this.options.experimental_prefetchInRender) { - this.#currentThenable.reject( - new Error('experimental_prefetchInRender feature flag is not enabled'), - ) - } - this.bindMethods() this.setOptions(options) } @@ -273,21 +263,21 @@ export class QueryObserver< result: QueryObserverResult, onPropTracked?: (key: keyof QueryObserverResult) => void, ): QueryObserverResult { - const handler: ProxyHandler> = { - get: (target, key: keyof QueryObserverResult) => { - if (key in target) { - this.trackProp(key) - if (onPropTracked) { - onPropTracked(key) - } - return target[key] - } - // Ensure all paths are covered by explicitly handling "undefined" properties - return undefined - }, - } + const trackedResult = {} as QueryObserverResult + + Object.keys(result).forEach((key) => { + Object.defineProperty(trackedResult, key, { + configurable: false, + enumerable: true, + get: () => { + this.trackProp(key as keyof QueryObserverResult) + onPropTracked?.(key as keyof QueryObserverResult) + return result[key as keyof QueryObserverResult] + }, + }) + }) - return new Proxy(result, handler) + return trackedResult } trackProp(key: keyof QueryObserverResult) { @@ -592,7 +582,6 @@ export class QueryObserver< isRefetchError: isError && hasData, isStale: isStale(query, options), refetch: this.refetch, - promise: this.#currentThenable, } return result as QueryObserverResult @@ -604,7 +593,6 @@ export class QueryObserver< | undefined const nextResult = this.createResult(this.#currentQuery, this.options) - this.#currentResultState = this.#currentQuery.state this.#currentResultOptions = this.options @@ -617,52 +605,6 @@ export class QueryObserver< return } - if (this.options.experimental_prefetchInRender) { - const finalizeThenableIfPossible = (thenable: PendingThenable) => { - if (nextResult.status === 'error') { - thenable.reject(nextResult.error) - } else if (nextResult.data !== undefined) { - thenable.resolve(nextResult.data) - } - } - - /** - * Create a new thenable and result promise when the results have changed - */ - const recreateThenable = () => { - const pending = - (this.#currentThenable = - nextResult.promise = - pendingThenable()) - - finalizeThenableIfPossible(pending) - } - - const prevThenable = this.#currentThenable - switch (prevThenable.status) { - case 'pending': - // Finalize the previous thenable if it was pending - finalizeThenableIfPossible(prevThenable) - break - case 'fulfilled': - if ( - nextResult.status === 'error' || - nextResult.data !== prevThenable.value - ) { - recreateThenable() - } - break - case 'rejected': - if ( - nextResult.status !== 'error' || - nextResult.error !== prevThenable.reason - ) { - recreateThenable() - } - break - } - } - this.#currentResult = nextResult // Determine which callbacks to trigger @@ -697,7 +639,6 @@ export class QueryObserver< return Object.keys(this.#currentResult).some((key) => { const typedKey = key as keyof QueryObserverResult const changed = this.#currentResult[typedKey] !== prevResult[typedKey] - return changed && includedProps.has(typedKey) }) } From a5e34729ce5be44e8e2666abe69cb83c050ba6a0 Mon Sep 17 00:00:00 2001 From: davidaghassi Date: Thu, 3 Oct 2024 10:15:08 -0700 Subject: [PATCH 4/4] refactor: packages/query-core/src/queryObserver.ts remove --- packages/query-core/src/queryObserver.ts | 67 ++++++++++++++++++++++-- 1 file changed, 63 insertions(+), 4 deletions(-) diff --git a/packages/query-core/src/queryObserver.ts b/packages/query-core/src/queryObserver.ts index 2971bc6d1a..37442ce42f 100644 --- a/packages/query-core/src/queryObserver.ts +++ b/packages/query-core/src/queryObserver.ts @@ -1,3 +1,8 @@ +import { focusManager } from './focusManager' +import { notifyManager } from './notifyManager' +import { fetchState } from './query' +import { Subscribable } from './subscribable' +import { pendingThenable } from './thenable' import { isServer, isValidTimeout, @@ -8,12 +13,9 @@ import { shallowEqualObjects, timeUntilStale, } from './utils' -import { notifyManager } from './notifyManager' -import { focusManager } from './focusManager' -import { Subscribable } from './subscribable' -import { fetchState } from './query' import type { FetchOptions, Query, QueryState } from './query' import type { QueryClient } from './queryClient' +import type { PendingThenable, Thenable } from './thenable' import type { DefaultError, DefaultedQueryObserverOptions, @@ -57,6 +59,7 @@ export class QueryObserver< TQueryData, TQueryKey > + #currentThenable: Thenable #selectError: TError | null #selectFn?: (data: TQueryData) => TData #selectResult?: TData @@ -82,6 +85,13 @@ export class QueryObserver< this.#client = client this.#selectError = null + this.#currentThenable = pendingThenable() + if (!this.options.experimental_prefetchInRender) { + this.#currentThenable.reject( + new Error('experimental_prefetchInRender feature flag is not enabled'), + ) + } + this.bindMethods() this.setOptions(options) } @@ -582,6 +592,7 @@ export class QueryObserver< isRefetchError: isError && hasData, isStale: isStale(query, options), refetch: this.refetch, + promise: this.#currentThenable, } return result as QueryObserverResult @@ -593,6 +604,7 @@ export class QueryObserver< | undefined const nextResult = this.createResult(this.#currentQuery, this.options) + this.#currentResultState = this.#currentQuery.state this.#currentResultOptions = this.options @@ -605,6 +617,52 @@ export class QueryObserver< return } + if (this.options.experimental_prefetchInRender) { + const finalizeThenableIfPossible = (thenable: PendingThenable) => { + if (nextResult.status === 'error') { + thenable.reject(nextResult.error) + } else if (nextResult.data !== undefined) { + thenable.resolve(nextResult.data) + } + } + + /** + * Create a new thenable and result promise when the results have changed + */ + const recreateThenable = () => { + const pending = + (this.#currentThenable = + nextResult.promise = + pendingThenable()) + + finalizeThenableIfPossible(pending) + } + + const prevThenable = this.#currentThenable + switch (prevThenable.status) { + case 'pending': + // Finalize the previous thenable if it was pending + finalizeThenableIfPossible(prevThenable) + break + case 'fulfilled': + if ( + nextResult.status === 'error' || + nextResult.data !== prevThenable.value + ) { + recreateThenable() + } + break + case 'rejected': + if ( + nextResult.status !== 'error' || + nextResult.error !== prevThenable.reason + ) { + recreateThenable() + } + break + } + } + this.#currentResult = nextResult // Determine which callbacks to trigger @@ -639,6 +697,7 @@ export class QueryObserver< return Object.keys(this.#currentResult).some((key) => { const typedKey = key as keyof QueryObserverResult const changed = this.#currentResult[typedKey] !== prevResult[typedKey] + return changed && includedProps.has(typedKey) }) }