Skip to content

perf(queriesObserver): fix O(n²) performance issue in batch updates#9467

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.

Sign up for GitHub

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

Closed
joseph0926 wants to merge 1 commit into from
Closed

perf(queriesObserver): fix O(n²) performance issue in batch updates #9467

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions packages/query-core/src/queriesObserver.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -44,6 +44,7 @@ export class QueriesObserver<
#lastCombine?: CombineFn<TCombinedResult>
#lastResult?: Array<QueryObserverResult>
#observerMatches: Array<QueryObserverMatch> = []
#indexMap: WeakMap<QueryObserver, number> = new WeakMap()

constructor(
client: QueryClient,
Expand DownExpand Up@@ -129,6 +130,11 @@ export class QueriesObserver<
this.#observers = newObservers
this.#result = newResult

this.#indexMap = new WeakMap()
newObservers.forEach((observer, index) => {
this.#indexMap.set(observer, index)
})

if (!this.hasListeners()) {
return
}
Expand DownExpand Up@@ -252,8 +258,8 @@ export class QueriesObserver<
}

#onUpdate(observer: QueryObserver, result: QueryObserverResult): void {
const index = this.#observers.indexOf(observer)
if (index !== -1) {
const index = this.#indexMap.get(observer)
if (index !== undefined) {
Comment on lines -255 to +262

@TkDodoTkDodoJul 31, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don’t we just pass the index into onUpdate instead of passing the whole observer and then trying to find the index again? For example:

diff --git a/packages/query-core/src/queriesObserver.ts b/packages/query-core/src/queriesObserver.ts
index 853e490ab..84defa874 100644
--- a/packages/query-core/src/queriesObserver.ts+++ b/packages/query-core/src/queriesObserver.ts@@ -63,9 +63,9 @@ export class QueriesObserver<
protected onSubscribe(): void {
if (this.listeners.size === 1) {
- this.#observers.forEach((observer) => {+ this.#observers.forEach((observer, index) => {
observer.subscribe((result) => {
- this.#onUpdate(observer, result)+ this.#onUpdate(index, result)
})
})
}
@@ -137,9 +137,9 @@ export class QueriesObserver<
observer.destroy()
})
- difference(newObservers, prevObservers).forEach((observer) => {+ difference(newObservers, prevObservers).forEach((observer, index) => {
observer.subscribe((result) => {
- this.#onUpdate(observer, result)+ this.#onUpdate(index, result)
})
})
@@ -251,12 +251,9 @@ export class QueriesObserver<
return observers
}
- #onUpdate(observer: QueryObserver, result: QueryObserverResult): void {- const index = this.#observers.indexOf(observer)- if (index !== -1) {- this.#result = replaceAt(this.#result, index, result)- this.#notify()- }+ #onUpdate(index: number, result: QueryObserverResult): void {+ this.#result = replaceAt(this.#result, index, result)+ this.#notify()
}
#notify(): void {

@joseph0926joseph0926Jul 31, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion
I considered passing the index directly, but noticed these potential issues

When setQueries() reorders observers, existing subscriptions keep their old captured indices

// Initial: observers = [A, B, C]// A subscribes with index=0 in onSubscribe()// After setQueries(): observers = [D, A, B, C] // A still uses index=0, but should use index=1

The index from difference().forEach() is not the actual position in this.#observers

difference(newObservers,prevObservers).forEach((observer,index)=>{// If difference returns [D, E], index is 0,1// But their actual positions in observers might be [A, D, B, E, C] -> 1,3observer.subscribe((result)=>{this.#onUpdate(index,result)// <- I think there is a problem here.})})

What do you think about these concerns?

@TkDodoTkDodoAug 1, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, can you then please add a test case where the implementation I suggested would fail.

@joseph0926joseph0926Aug 1, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

import{afterEach,beforeEach,describe,expect,test,vi}from'vitest'import{queryKey}from'@tanstack/query-test-utils'import{QueriesObserver,QueryClient}from'..'importtype{QueryObserverResult}from'..'describe('queriesObserver - index tracking issue',()=>{letqueryClient: QueryClientbeforeEach(()=>{vi.useFakeTimers()queryClient=newQueryClient()queryClient.mount()})afterEach(()=>{queryClient.clear()vi.useRealTimers()})test('should fail when using forEach index with dynamic query changes',async()=>{constkey1=queryKey()constkey2=queryKey()constkey3=queryKey()constkey4=queryKey()constqueryFn1=vi.fn().mockReturnValue('data1')constqueryFn2=vi.fn().mockReturnValue('data2')constqueryFn3=vi.fn().mockReturnValue('data3')constqueryFn4=vi.fn().mockReturnValue('data4')constobserver=newQueriesObserver(queryClient,[{queryKey: key1,queryFn: queryFn1},{queryKey: key2,queryFn: queryFn2},])constresults: Array<Array<QueryObserverResult>>=[]constunsubscribe=observer.subscribe((result)=>{results.push([...result])})awaitvi.advanceTimersByTimeAsync(0)results.length=0observer.setQueries([{queryKey: key3,queryFn: queryFn3},{queryKey: key1,queryFn: queryFn1},{queryKey: key4,queryFn: queryFn4},{queryKey: key2,queryFn: queryFn2},])awaitvi.advanceTimersByTimeAsync(0)queryClient.setQueryData(key3,'updated3')queryClient.setQueryData(key4,'updated4')queryClient.setQueryData(key1,'updated1')unsubscribe()constfinalResult=results[results.length-1]expect(finalResult).toHaveLength(4)expect(finalResult?.[0]).toMatchObject({data: 'updated3'})expect(finalResult?.[1]).toMatchObject({data: 'updated1'})expect(finalResult?.[2]).toMatchObject({data: 'updated4'})expect(finalResult?.[3]).toMatchObject({data: 'data2'})})test('should fail when reordering queries with existing subscriptions',async()=>{constkey1=queryKey()constkey2=queryKey()constkey3=queryKey()letupdateCount=0constqueryFn1=vi.fn().mockImplementation(()=>`data1-${++updateCount}`)constqueryFn2=vi.fn().mockImplementation(()=>`data2-${++updateCount}`)constqueryFn3=vi.fn().mockImplementation(()=>`data3-${++updateCount}`)constobserver=newQueriesObserver(queryClient,[{queryKey: key1,queryFn: queryFn1},{queryKey: key2,queryFn: queryFn2},{queryKey: key3,queryFn: queryFn3},])constresults: Array<Array<QueryObserverResult>>=[]constunsubscribe=observer.subscribe((result)=>{results.push([...result])})awaitvi.advanceTimersByTimeAsync(0)results.length=0observer.setQueries([{queryKey: key3,queryFn: queryFn3},{queryKey: key1,queryFn: queryFn1},{queryKey: key2,queryFn: queryFn2},])awaitqueryClient.invalidateQueries({queryKey: key1})awaitvi.advanceTimersByTimeAsync(0)constresultAfterInvalidate=results[results.length-1]expect(resultAfterInvalidate?.[1]?.data).toMatch(/data1-\d+/)expect(resultAfterInvalidate?.[0]?.data).toBe('data3-3')expect(resultAfterInvalidate?.[2]?.data).toBe('data2-2')unsubscribe()})})
queries-test

the failure only occurs with the version that captures the index in the subscription callback. When I run the same tests against either the current main branch’s queriesObserver.ts or the queriesObserver.ts in my PR, all of the tests pass without any issues.

@TkDodoTkDodoAug 2, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you, but then please add the test case to the PR.

@joseph0926joseph0926Aug 3, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following your suggestion, I've created and run various performance tests, and I'd like to share the results. While theoretically replacing indexOf O(n) with WeakMap O(1) should improve performance, the actual test results were different from what I expected.

  • Even in extreme cases with 10,000 queries, there was no meaningful performance difference
  • In some cases, the WeakMap version was actually slightly slower

It seems that other optimizations like using Set in the difference function and caching in findMatchingObservers have already resolved the main O(n²) issues.
I apologize for taking up your valuable time reviewing a PR that ultimately doesn't provide substantial improvements. I would appreciate it if you could close the PR.
This has been a great learning experience, and I'm grateful for your time and feedback. Thank you for the opportunity to contribute to the project.

@joseph0926joseph0926Aug 3, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, I am sharing one of the codes I tested.

import{afterEach,beforeEach,describe,expect,test}from'vitest'import{QueriesObserver,QueryClient}from'..'importtype{QueryObserverOptions}from'..'describe('QueriesObserver extreme cases - Real-time Dashboard',()=>{letqueryClient: QueryClientbeforeEach(()=>{queryClient=newQueryClient({defaultOptions: {queries: {retry: false,},},})queryClient.mount()})afterEach(()=>{queryClient.clear()})test('should handle real-time dashboard with 10,000+ queries where last ones update frequently',async()=>{constTOTAL_QUERIES=10000constACTIVE_QUERIES=100constUPDATE_ROUNDS=10console.log(`\n Real-time Dashboard Simulation:`)console.log(`Total queries: ${TOTAL_QUERIES}`)console.log(`Active queries (frequently updating): ${ACTIVE_QUERIES}`)console.log(`Update rounds: ${UPDATE_ROUNDS}\n`)constcreateQueries=(round: number=0): Array<QueryObserverOptions>=>{returnArray.from({length: TOTAL_QUERIES},(_,i)=>{constisActive=i>=TOTAL_QUERIES-ACTIVE_QUERIESreturn{queryKey: ['dashboard',i,round],queryFn: async()=>{awaitnewPromise((resolve)=>setTimeout(resolve,0))return{id: i,data: `${round===0 ? 'initial' : `update-${round}`}-${i}`,timestamp: Date.now(),
isActive,}},staleTime: isActive ? 0 : Infinity,}})}conststartSetup=performance.now()constobserver=newQueriesObserver(queryClient,createQueries(0))lettotalUpdates=0constupdateTimes: Array<number>=[]letlastUpdateTime=performance.now()constupdateMetrics: Array<{round: numberupdateCount: numberduration: numberavgTimePerUpdate: number}>=[]constunsubscribe=observer.subscribe(()=>{constnow=performance.now()updateTimes.push(now-lastUpdateTime)lastUpdateTime=nowtotalUpdates++})constsetupTime=performance.now()-startSetupconsole.log(`Setup time: ${setupTime.toFixed(1)}ms`)console.log('\n Initial load...')constinitialLoadStart=performance.now()awaitnewPromise((resolve)=>setTimeout(resolve,100))constinitialLoadTime=performance.now()-initialLoadStartconstinitialUpdateCount=totalUpdatesconsole.log(`Initial load completed: ${initialLoadTime.toFixed(1)}ms, ${initialUpdateCount} updates`,)console.log('\n Starting frequent updates on active queries...')for(letround=1;round<=UPDATE_ROUNDS;round++){constroundStart=performance.now()constupdateCountBefore=totalUpdatesconstupdatedQueries=Array.from({length: TOTAL_QUERIES},(_,i)=>{constisActive=i>=TOTAL_QUERIES-ACTIVE_QUERIESif(isActive){return{queryKey: ['dashboard',i,round],queryFn: async()=>{awaitnewPromise((resolve)=>setTimeout(resolve,0))return{id: i,data: `update-${round}-${i}`,timestamp: Date.now(),isActive: true,}},staleTime: 0,}}else{return{queryKey: ['dashboard',i,0],queryFn: async()=>{return{id: i,data: `initial-${i}`,timestamp: Date.now(),isActive: false,}},staleTime: Infinity,}}})observer.setQueries(updatedQueries)awaitnewPromise((resolve)=>setTimeout(resolve,50))constroundDuration=performance.now()-roundStartconstroundUpdates=totalUpdates-updateCountBeforeupdateMetrics.push({
round,updateCount: roundUpdates,duration: roundDuration,avgTimePerUpdate: roundDuration/ACTIVE_QUERIES,})}console.log('\n Performance Analysis:')console.log('Round | Duration | Updates | Avg/Update')console.log('------|----------|---------|------------')updateMetrics.forEach(({ round, duration, updateCount, avgTimePerUpdate })=>{console.log(`${round.toString().padEnd(5)} | ${duration.toFixed(1).padEnd(8)}ms | ${updateCount.toString().padEnd(7)} | ${avgTimePerUpdate.toFixed(2)}ms`,)},)constvalidMetrics=updateMetrics.filter((m)=>m.updateCount>0)if(validMetrics.length>=2){constfirstRoundAvg=validMetrics[0].avgTimePerUpdateconstlastRoundAvg=validMetrics[validMetrics.length-1].avgTimePerUpdateconstdegradation=lastRoundAvg/firstRoundAvgconsole.log(`\n Performance degradation: ${degradation.toFixed(2)}x`)console.log(`Total updates processed: ${totalUpdates}`)if(updateTimes.length>100){constsortedTimes=[...updateTimes].sort((a,b)=>a-b)constp50=sortedTimes[Math.floor(sortedTimes.length*0.5)]constp95=sortedTimes[Math.floor(sortedTimes.length*0.95)]constp99=sortedTimes[Math.floor(sortedTimes.length*0.99)]console.log('\n Update time distribution:')console.log(`P50: ${p50?.toFixed(2)}ms`)console.log(`P95: ${p95?.toFixed(2)}ms`)console.log(`P99: ${p99?.toFixed(2)}ms`)}expect(degradation).toBeLessThan(3.0)}unsubscribe()})test('should demonstrate O(n) vs O(n²) behavior with increasing observer counts',async()=>{consttestSizes=[1000,2000,4000,8000]constresults: Array<{size: numbersetupTime: numberupdateTime: numbertimePerQuery: number}>=[]console.log('\n Scalability Analysis:')for(constsizeoftestSizes){constactiveCount=Math.min(100,size/10)constqueries=Array.from({length: size},(_,i)=>({queryKey: ['scale-test',size,i],queryFn: async()=>({id: i,data: `result-${i}`}),staleTime: i>=size-activeCount ? 0 : Infinity,}))constsetupStart=performance.now()constobserver=newQueriesObserver(queryClient,queries)letupdateCount=0constunsubscribe=observer.subscribe(()=>{updateCount++})constsetupTime=performance.now()-setupStartawaitnewPromise((resolve)=>setTimeout(resolve,50))constupdateStart=performance.now()constupdatedQueries=queries.map((q,i)=>{if(i>=size-activeCount){return{
...q,queryKey: ['scale-test',size,i,'updated'],}}returnq})observer.setQueries(updatedQueries)awaitnewPromise((resolve)=>setTimeout(resolve,50))constupdateTime=performance.now()-updateStartresults.push({
size,
setupTime,
updateTime,timePerQuery: updateTime/activeCount,})unsubscribe()awaitqueryClient.clear()}console.log('Size | Setup Time | Update Time | Time/Query | Growth')console.log('------|------------|-------------|------------|--------')results.forEach((result,i)=>{constgrowth=i>0
? (result.timePerQuery/results[0].timePerQuery).toFixed(2)
: '1.00'console.log(`${result.size.toString().padEnd(5)} | ${result.setupTime.toFixed(1).padEnd(10)}ms | ${result.updateTime.toFixed(1).padEnd(11)}ms | ${result.timePerQuery.toFixed(2).padEnd(10)}ms | ${growth}x`,)})consttimePerQueryRatios=results.map((r)=>r.timePerQuery)constmaxRatio=Math.max(...timePerQueryRatios)constminRatio=Math.min(...timePerQueryRatios)constvarianceRatio=maxRatio/minRatioconsole.log(`\nTime per query variance: ${varianceRatio.toFixed(2)}x (should be close to 1.0 for O(n))`,)expect(varianceRatio).toBeLessThan(3.0)})})

@TkDodoTkDodoAug 4, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright, let’s close this then. Thanks for working on this 🙏

this.#result = replaceAt(this.#result, index, result)
this.#notify()
}
Expand Down
Loading