-
Notifications
You must be signed in to change notification settings - Fork 505
fix: add combined size pool #1848
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,12 @@ type RequestPool = { | |
| * You don't need to directly use the imageRetrievalPoolManager to load images | ||
| * since the imageLoadPoolManager will automatically use it for retrieval. However, | ||
| * maximum number of concurrent requests can be set by calling `setMaxConcurrentRequests`. | ||
| * | ||
| * ### Requests between types | ||
| * There are different types of requests, and each type starves the next request | ||
| * type, under the assumption that they are needed in order for fetching. | ||
| * The total number of requests is set by the setMaxConcurrentRequests, defaulting to 50 | ||
| * concurrent requests. | ||
| */ | ||
| class RequestPoolManager { | ||
| private id: string; | ||
|
|
@@ -77,8 +83,11 @@ class RequestPoolManager { | |
| [RequestType.Prefetch]: 0, | ||
| [RequestType.Compute]: 0, | ||
| } as Record<RequestType, number>; | ||
| private maxConcurrentRequests = 50; | ||
|
|
||
| /* maximum number of requests of each type. */ | ||
| public maxNumRequests: { | ||
| [RequestType.Metadata]: number; | ||
| [RequestType.Interaction]: number; | ||
| [RequestType.Thumbnail]: number; | ||
| [RequestType.Prefetch]: number; | ||
|
|
@@ -97,6 +106,7 @@ class RequestPoolManager { | |
| this.id = id ? id : uuidv4(); | ||
|
|
||
| this.requestPool = { | ||
| [RequestType.Metadata]: { 0: [] }, | ||
| [RequestType.Interaction]: { 0: [] }, | ||
| [RequestType.Thumbnail]: { 0: [] }, | ||
| [RequestType.Prefetch]: { 0: [] }, | ||
|
|
@@ -107,13 +117,15 @@ class RequestPoolManager { | |
| this.awake = false; | ||
|
|
||
| this.numRequests = { | ||
| [RequestType.Metadata]: 0, | ||
| [RequestType.Interaction]: 0, | ||
| [RequestType.Thumbnail]: 0, | ||
| [RequestType.Prefetch]: 0, | ||
| [RequestType.Compute]: 0, | ||
| } as Record<RequestType, number>; | ||
|
|
||
| this.maxNumRequests = { | ||
| [RequestType.Metadata]: 6, | ||
| [RequestType.Interaction]: 6, | ||
| [RequestType.Thumbnail]: 6, | ||
| [RequestType.Prefetch]: 5, | ||
|
|
@@ -127,6 +139,14 @@ class RequestPoolManager { | |
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the overall maximum number of requests combined between the various types. | ||
| * Only applies to metadata, interaction, thumbnail and prefetch | ||
| */ | ||
| public setMaxConcurrentRequests(maxNumRequests: number) { | ||
| this.maxConcurrentRequests = maxNumRequests; | ||
| } | ||
|
|
||
| /** | ||
| * This function sets the maximum number of requests for a given request type. | ||
| * @param type - The type of request you want to set the max number | ||
|
|
@@ -231,8 +251,18 @@ class RequestPoolManager { | |
| this.requestPool[type] = { 0: [] }; | ||
| } | ||
|
|
||
| private sendRequests(type) { | ||
| const requestsToSend = this.maxNumRequests[type] - this.numRequests[type]; | ||
| protected getRequestsAvailable(type) { | ||
| return this.maxNumRequests[type] - this.numRequests[type]; | ||
| } | ||
|
|
||
| private sendRequests(type, maxRemaining = 1000) { | ||
| if (maxRemaining <= 0) { | ||
| return this.getRequestsAvailable(type) > 0; | ||
| } | ||
| const requestsToSend = Math.min( | ||
| this.getRequestsAvailable(type), | ||
| maxRemaining | ||
| ); | ||
| let syncImageCount = 0; | ||
|
|
||
| for (let i = 0; i < requestsToSend; i++) { | ||
|
|
@@ -280,19 +310,50 @@ class RequestPoolManager { | |
| return null; | ||
| } | ||
|
|
||
| public get outstandingRequests() { | ||
| return ( | ||
| this.numRequests[RequestType.Interaction] + | ||
| this.numRequests[RequestType.Thumbnail] + | ||
| this.numRequests[RequestType.Prefetch] | ||
| ); | ||
| } | ||
|
|
||
| protected startGrabbing(): void { | ||
| const hasRemainingMetadataRequests = this.sendRequests( | ||
| RequestType.Metadata, | ||
| this.maxConcurrentRequests - this.outstandingRequests | ||
| ); | ||
| let available = this.maxConcurrentRequests - this.outstandingRequests; | ||
| // The && available after checking for > 0 means it uses the available requests as a positive value | ||
| // Then, allow 1 interaction request always | ||
| // Only allow 0 requests for other types - this is the || 0 part to get a 0 result rather than a false one | ||
| const hasRemainingInteractionRequests = this.sendRequests( | ||
| RequestType.Interaction | ||
| RequestType.Interaction, | ||
| (available > 0 && available) || | ||
| (this.numRequests[RequestType.Interaction] === 0 && 1) || | ||
| 0 | ||
| ); | ||
| available = this.maxConcurrentRequests - this.outstandingRequests; | ||
| // Allow an extra request for thumbnail if there are no outstanding | ||
| // interaction or thumbnail requests. | ||
| const hasRemainingThumbnailRequests = this.sendRequests( | ||
| RequestType.Thumbnail | ||
| RequestType.Thumbnail, | ||
| (available > 0 && available) || | ||
|
wayfarer3130 marked this conversation as resolved.
|
||
| (this.numRequests[RequestType.Interaction] === 0 && | ||
| this.numRequests[RequestType.Thumbnail] === 0 && | ||
| 1) || | ||
| 0 | ||
| ); | ||
| const hasRemainingPrefetchRequests = this.sendRequests( | ||
| RequestType.Prefetch | ||
| RequestType.Prefetch, | ||
| this.maxConcurrentRequests - this.outstandingRequests | ||
| ); | ||
|
|
||
| // Compute requests are on a different queue as they don't use http requests | ||
| const hasRemainingComputeRequests = this.sendRequests(RequestType.Compute); | ||
|
|
||
| if ( | ||
| !hasRemainingMetadataRequests && | ||
|
Comment on lines
+313
to
+356
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant region.
ast-grep outline packages/core/src/requestPool/requestPoolManager.ts --view expanded || true
wc -l packages/core/src/requestPool/requestPoolManager.ts
sed -n '1,220p' packages/core/src/requestPool/requestPoolManager.ts
sed -n '220,420p' packages/core/src/requestPool/requestPoolManager.tsRepository: cornerstonejs/cornerstone3D Length of output: 15232 Count Metadata in the combined request cap. 🤖 Prompt for AI Agents |
||
| !hasRemainingInteractionRequests && | ||
| !hasRemainingThumbnailRequests && | ||
| !hasRemainingPrefetchRequests && | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| import { RequestPoolManager } from '../src/requestPool/requestPoolManager'; | ||
| import RequestType from '../src/enums/RequestType'; | ||
|
|
||
| /** | ||
| * Builds a request function that returns a promise which never settles, so the | ||
| * pool keeps counting it as "in flight". Each call is recorded on `requestFn` | ||
| * (a jest.fn) so tests can assert exactly how many requests were dispatched. | ||
| */ | ||
| function pendingRequest() { | ||
| return jest.fn(() => new Promise(() => {})); | ||
| } | ||
|
|
||
| /** | ||
| * Adds `count` requests of a given type, each backed by its own pending | ||
| * requestFn, and returns the array of jest.fns so the caller can count how | ||
| * many were actually invoked (i.e. dispatched) by the pool. | ||
| */ | ||
| function addPending(manager, type, count, priority = 0) { | ||
| const fns = []; | ||
| for (let i = 0; i < count; i++) { | ||
| const fn = pendingRequest(); | ||
| fns.push(fn); | ||
| manager.addRequest(fn, type, {}, priority); | ||
| } | ||
| return fns; | ||
| } | ||
|
|
||
| function dispatchedCount(fns) { | ||
| return fns.filter((fn) => fn.mock.calls.length > 0).length; | ||
| } | ||
|
|
||
| describe('RequestPoolManager', () => { | ||
| describe('default configuration', () => { | ||
| it('includes the Metadata type in the per-type maximums', () => { | ||
| const manager = new RequestPoolManager(); | ||
|
|
||
| // Metadata was added alongside the combined pool; it defaults to the same | ||
| // per-type ceiling as interaction/thumbnail. | ||
| expect(manager.getMaxSimultaneousRequests(RequestType.Metadata)).toBe(6); | ||
| expect(manager.getMaxSimultaneousRequests(RequestType.Interaction)).toBe( | ||
| 6 | ||
| ); | ||
| expect(manager.getMaxSimultaneousRequests(RequestType.Thumbnail)).toBe(6); | ||
| expect(manager.getMaxSimultaneousRequests(RequestType.Prefetch)).toBe(5); | ||
| expect(manager.getMaxSimultaneousRequests(RequestType.Compute)).toBe( | ||
| 1000 | ||
| ); | ||
| }); | ||
|
|
||
| it('starts with no outstanding requests', () => { | ||
| const manager = new RequestPoolManager(); | ||
| expect(manager.outstandingRequests).toBe(0); | ||
| }); | ||
| }); | ||
|
|
||
| describe('combined pool limit', () => { | ||
| it('caps the total in-flight requests across types at maxConcurrentRequests', () => { | ||
| const manager = new RequestPoolManager(); | ||
| manager.setMaxConcurrentRequests(3); | ||
|
|
||
| // Prefetch alone would allow 5 concurrent (its per-type max), but the | ||
| // combined pool of 3 must win. | ||
| const fns = addPending(manager, RequestType.Prefetch, 10); | ||
|
|
||
| expect(dispatchedCount(fns)).toBe(3); | ||
| expect(manager.outstandingRequests).toBe(3); | ||
| }); | ||
|
|
||
| it('is bounded by the per-type maximum when it is lower than the combined pool', () => { | ||
| const manager = new RequestPoolManager(); | ||
| manager.setMaxConcurrentRequests(20); | ||
|
|
||
| // Combined pool allows 20, but prefetch is capped at 5 per type. | ||
| const fns = addPending(manager, RequestType.Prefetch, 10); | ||
|
|
||
| expect(dispatchedCount(fns)).toBe(5); | ||
| expect(manager.outstandingRequests).toBe(5); | ||
| }); | ||
| }); | ||
|
|
||
| describe('interaction is never starved by lower-priority fetches', () => { | ||
| it('still dispatches one interaction request when the combined pool is full', () => { | ||
| const manager = new RequestPoolManager(); | ||
| manager.setMaxConcurrentRequests(2); | ||
|
|
||
| // Saturate the combined pool with prefetch (lower priority) work. | ||
| const prefetchFns = addPending(manager, RequestType.Prefetch, 5); | ||
| expect(dispatchedCount(prefetchFns)).toBe(2); | ||
| expect(manager.outstandingRequests).toBe(2); | ||
|
|
||
| // An interaction arriving after the pool is full must still get through - | ||
| // this is the guarantee that background fetches cannot block interaction. | ||
| const interactionFns = addPending(manager, RequestType.Interaction, 3); | ||
|
|
||
| expect(dispatchedCount(interactionFns)).toBe(1); | ||
| }); | ||
|
|
||
| it('only forces through a single interaction, not the whole backlog', () => { | ||
| const manager = new RequestPoolManager(); | ||
| manager.setMaxConcurrentRequests(2); | ||
|
|
||
| addPending(manager, RequestType.Prefetch, 5); | ||
| const interactionFns = addPending(manager, RequestType.Interaction, 4); | ||
|
|
||
| // With one interaction already in flight and the pool over capacity, the | ||
| // remaining interaction requests wait their turn. | ||
| expect(dispatchedCount(interactionFns)).toBe(1); | ||
| }); | ||
| }); | ||
|
|
||
| describe('outstandingRequests', () => { | ||
| it('counts interaction, thumbnail and prefetch but excludes metadata', () => { | ||
| const manager = new RequestPoolManager(); | ||
|
|
||
| // A metadata request is in flight... | ||
| const metadataFns = addPending(manager, RequestType.Metadata, 1); | ||
| expect(dispatchedCount(metadataFns)).toBe(1); | ||
| // ...but it does not count toward outstandingRequests. | ||
| expect(manager.outstandingRequests).toBe(0); | ||
|
|
||
| // A prefetch request does count. | ||
| const prefetchFns = addPending(manager, RequestType.Prefetch, 1); | ||
| expect(dispatchedCount(prefetchFns)).toBe(1); | ||
| expect(manager.outstandingRequests).toBe(1); | ||
| }); | ||
| }); | ||
|
|
||
| describe('compute queue', () => { | ||
| it('runs on a separate queue that ignores the combined pool limit', () => { | ||
| const manager = new RequestPoolManager(); | ||
| manager.setMaxConcurrentRequests(1); | ||
|
|
||
| // Compute requests do not use HTTP requests, so they are dispatched off a | ||
| // separate queue that the combined pool size does not throttle. | ||
| const fns = addPending(manager, RequestType.Compute, 5); | ||
|
|
||
| expect(dispatchedCount(fns)).toBe(5); | ||
| // And they are not reflected in outstandingRequests. | ||
| expect(manager.outstandingRequests).toBe(0); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,6 +82,40 @@ imageLoadPoolManager.addRequest( | |
| ); | ||
| ``` | ||
|
|
||
| ## Combined concurrent request limit | ||
|
|
||
| In addition to the per-type maximums above, each pool enforces a **combined | ||
| cap** on the total number of concurrent HTTP requests across all types | ||
| (`metadata`, `interaction`, `thumbnail`, and `prefetch`). This prevents | ||
| lower-priority work (such as a large prefetch backlog) from saturating the | ||
| browser's connection pool and starving interaction fetches. | ||
|
|
||
| The combined cap defaults to `50` and can be changed with | ||
| `setMaxConcurrentRequests`: | ||
|
|
||
| ```js | ||
| import { imageLoadPoolManager } from '@cornerstonejs/core'; | ||
|
|
||
| // Total concurrent HTTP requests across all types for this pool. | ||
| imageLoadPoolManager.setMaxConcurrentRequests(100); | ||
| ``` | ||
|
|
||
| The number of requests actually dispatched for a given type is the smaller of | ||
| its per-type maximum (`setMaxSimultaneousRequests`) and the remaining combined | ||
| budget. Even when the combined pool is full, at least one `Interaction` request | ||
| is always allowed through so background fetches can never fully block | ||
| interaction. | ||
|
Comment on lines
+87
to
+107
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Resolve the combined-concurrency contract mismatch. The documentation promises a strict cap across metadata and image requests, but the supplied scheduler excludes in-flight metadata from
📍 Affects 2 files
🤖 Prompt for AI Agents |
||
|
|
||
| The request types, in priority order, are: | ||
|
|
||
| | Type | Purpose | | ||
| | ------------- | -------------------------------------------------------------- | | ||
| | `Metadata` | Metadata that must resolve before images can render (highest). | | ||
| | `Interaction` | Images needed for the current interaction. | | ||
| | `Thumbnail` | Thumbnail images. | | ||
| | `Prefetch` | Background image loading. | | ||
| | `Compute` | Non-HTTP compute work; runs on a separate, un-throttled queue. | | ||
|
|
||
| ## Requests re-ordering | ||
|
|
||
| You could have a certain sequence in mind for retrieving the images. For example, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.