Conversation
9b050f9 to
b75c6d2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6113 +/- ##
=======================================
Coverage 70.58% 70.58%
=======================================
Files 409 409
Lines 109672 109685 +13
Branches 18070 18074 +4
=======================================
+ Hits 77408 77425 +17
+ Misses 21445 21443 -2
+ Partials 10819 10817 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jasnell
reviewed
Feb 19, 2026
jasnell
approved these changes
Feb 19, 2026
22119db to
0974e57
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Add INCREASE_EXTERNAL_MEMORY_ADJUSTMENT_FOR_FETCH autogate that increases the external memory adjustment for fetch() from 3 KiB to 8 KiB. This brings it closer to the actual C++ memory overhead. Also removes the fully-rolled-out FETCH_REQUEST_MEMORY_ADJUSTMENT autogate, making the 3 KiB adjustment unconditional by default. Release note: None Co-Authored-By: Claude <noreply@anthropic.com>
When the INCREASE_EXTERNAL_MEMORY_ADJUSTMENT_FOR_FETCH autogate is enabled, apply an 8 KiB external memory adjustment to all concurrency-limited subrequests (fetch, KV, R2, Cache, Queues, Hyperdrive, TCP, WebSocket), not just fetch(). This is done centrally in IoContext::getSubrequestNoChecks() and IoContext::getCacheClient() using IoContext::currentLock to obtain the jsg::Lock without requiring any signature changes. When the autogate is off, the existing 3 KiB adjustment remains in fetchImplNoOutputLock() for fetch() only, preserving the old behavior. Co-Authored-By: Claude <noreply@anthropic.com>
0974e57 to
ffed0c5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR:
INCREASE_EXTERNAL_MEMORY_ADJUSTMENT_FOR_FETCH.FETCH_REQUEST_MEMORY_ADJUSTMENT.fetch()es from 3 KiB to 8 KiB when the new autogate is on.IoContext::getSubrequestNoChecks()so that it covers all subrequests, not justfetch()subrequests.In testing, I have discovered that the real C++ heap overhead of
fetch()is 8 KiB if it ends up being queued in our concurrent connection limiter, and 32 KiB if it manages to establish a connection. So, this change brings the external memory adjustment closer to the actual unaccounted-for memory, regardless of whether or not thefetch()is queued. Also note that other factors, such as header size, could increase the overhead significantly higher, but probably don't much influence the mean or median overhead.Moving the external memory adjustment from just
fetch()to all subrequests is probably the most consequential part of this change, and worth discussing.The dilemma is this: We'd like to relax the concurrent connection limiting behavior and rely solely on heap pressure to force the Worker to keep its concurrent subrequest connection count under control. So, it makes sense to apply this external memory adjustment to all subrequest types which end up establishing a network connection, not just
fetch(). But, we can't know if a subrequest will actually need to establish a network connection until much deeper in our internal integration, where it would be an abstraction leak to add an external memory adjustment. This is already even true forfetch()itself -- it may invoke a Service Binding and not go over the network, after all.I think expanding the external memory adjustment to cover all subrequest types is the right thing to do largely because all of the alternatives seem worse: We don't want to leave any potential network subrequests unaccounted for; we don't want maintain a a brittle, whackamole solution early in the call graph (close to
fetch(), etc.); and we don't want to implement an abstraction leak later in the call graph (close to the actual network connection).In a more positive light, I would also argue that applying the memory adjustment to subrequests unconditionally has the nice property that it applies pressure on the root originator of pressure in the system: if a Worker issues 100
fetch()es to 100 different service bindings, each of which then establishes one network connection, the root Worker receives the majority of the memory pressure to constrain its concurrency.