fix: add combined size pool#1848
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| const hasRemainingThumbnailRequests = this.sendRequests( | ||
| RequestType.Thumbnail | ||
| RequestType.Thumbnail, | ||
| (available > 0 && available) || |
There was a problem hiding this comment.
Can't this be simplied to (available > 0)?
There was a problem hiding this comment.
No - added a comment to explain that.
jbocce
left a comment
jbocce
left a comment
There was a problem hiding this comment.
Couple of things to look at. Thanks.
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, comment @claude review on this pull request to trigger a review.
|
FYI... the OHIF downstream validation is failing. |
feat: add combined size pool to overall request pool Re-applies the combined request-pool limit onto main. Adds a Metadata request type and an overall maxConcurrentRequests cap shared across the metadata/interaction/thumbnail/prefetch queues, so lower-priority fetches cannot starve interaction fetches (interaction is always allowed at least one request). Adds jest unit tests covering the combined cap, interaction guarantee, outstandingRequests accounting, and the separate compute queue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> @
wayfarer3130
force-pushed
the
feat/add-combined-size-pool
branch
from
6f854ed to
cb6d526
Compare
📝 WalkthroughWalkthrough
ChangesRequest scheduling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RequestPoolManager
participant MetadataQueue
participant InteractionQueue
participant ComputeQueue
RequestPoolManager->>MetadataQueue: dispatch metadata within remaining capacity
RequestPoolManager->>InteractionQueue: dispatch interaction with priority allowance
RequestPoolManager->>ComputeQueue: dispatch compute independently
RequestPoolManager-->>RequestPoolManager: update outstanding request count
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/test/requestPoolManager.jest.js (1)
56-79: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test mixing Metadata with another type under the combined cap.
Existing "combined pool limit" tests only use a single type (Prefetch), so they don't catch the Metadata-exclusion issue flagged in
requestPoolManager.ts(Lines 313-356) where Metadata's in-flight count isn't deducted from the budget seen by other types (and vice versa), letting the total exceedmaxConcurrentRequests.it('deducts in-flight Metadata requests from the combined pool available to other types', () => { const manager = new RequestPoolManager(); manager.setMaxConcurrentRequests(5); const metadataFns = addPending(manager, RequestType.Metadata, 5); expect(dispatchedCount(metadataFns)).toBe(5); // Combined pool of 5 is already fully consumed by Metadata. const prefetchFns = addPending(manager, RequestType.Prefetch, 3); expect(dispatchedCount(prefetchFns)).toBe(0); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/requestPoolManager.jest.js` around lines 56 - 79, Extend the “combined pool limit” tests with a Metadata-plus-Prefetch regression case: configure maxConcurrentRequests to 5, dispatch five Metadata requests, then add three Prefetch requests and assert none dispatch. Use the existing RequestPoolManager, addPending, RequestType, and dispatchedCount helpers without changing production code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/requestPool/requestPoolManager.ts`:
- Around line 313-356: Update the outstandingRequests getter to include
this.numRequests[RequestType.Metadata] in the combined in-flight request count.
Preserve the existing startGrabbing() cap calculations so metadata reduces the
available budget for interaction, thumbnail, and prefetch requests.
---
Nitpick comments:
In `@packages/core/test/requestPoolManager.jest.js`:
- Around line 56-79: Extend the “combined pool limit” tests with a
Metadata-plus-Prefetch regression case: configure maxConcurrentRequests to 5,
dispatch five Metadata requests, then add three Prefetch requests and assert
none dispatch. Use the existing RequestPoolManager, addPending, RequestType, and
dispatchedCount helpers without changing production code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d55ca82a-8e96-4bca-8164-2fc05324a0e3
📒 Files selected for processing (3)
packages/core/src/enums/RequestType.tspackages/core/src/requestPool/requestPoolManager.tspackages/core/test/requestPoolManager.jest.js
| 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) || | ||
| (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 && |
There was a problem hiding this comment.
🎯 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. outstandingRequests excludes RequestType.Metadata, but startGrabbing() uses it for every cap calculation. That lets metadata run without reducing the budget for interaction/thumbnail/prefetch, so total in-flight requests can exceed maxConcurrentRequests (for example, 6 metadata + 6 interaction + 4 thumbnail with defaults). Include metadata in the combined gating.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/requestPool/requestPoolManager.ts` around lines 313 - 356,
Update the outstandingRequests getter to include
this.numRequests[RequestType.Metadata] in the combined in-flight request count.
Preserve the existing startGrabbing() cap calculations so metadata reduces the
available budget for interaction, thumbnail, and prefetch requests.
2 participants
Context
This PR lays the groundwork for adding display fetch controls that delay initial fetching of thumbnails and prefetches until the primary viewports are completed.
Changes & Results
Adds a prefetch queue for metadata
Adds an overall maximum size prefetch queue that applies to the metadata through prefetch queues, with the higher priority queues being done first. The intent of this is in an application such as OHIF to allow all the metadata queries to be sent off initially, and then let those continue to come in, while the initial images are not fetched initially. Then, the viewport specific images would be fetched and then thumbnails and finally prefetch images.
Testing
Link into OHIF, and display a study with thumbnails and prefetch. Add a log message on prefetch queue.
Reduce the delay in study browser to 125 ms or something like that, and all the thumbnails should be before the prefetch.
Should also add the metadata prefetch to the queue at level metadata, and add a viewport fetch for first image at low priority as soon as the display sets are created. That will demonstrate slowly fetching viewport/thumbnail images mixed in with metadata requests.
Checklist
PR
semantic-release format and guidelines.
Code
[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the
yarn build:update-apito update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
New Features
Bug Fixes