Repository navigation
KTOR-10131 Server core and utils allocation improvements (split from #5976) - #5983
Diogo Martins (MDA2AV) wants to merge 10 commits into
Conversation
…d lock only when concurrent, as on the JVM; an AttributeKey hashes once; a pipeline keeps its interceptors in a volatile field and reads the shared flags it wrote on every execute
…y when something asks for them, routing decodes no query its handler does not read (one with a bad escape still fails the call), and a call allocates no Lazy or atomic box
…d is a volatile load and no lock, where a route-scoped plugin looked itself up on every call through ConcurrentMap's lock, which every reactor took turns on (ktor-kronos-native json-tls at 64 reactors locally: 103K req/s at 1170% CPU, now 170K at 1574%)
…, where it is written - compressed() was given no context, so its writer went to Dispatchers.Default and every compressed response crossed threads twice (ktor-kronos json-comp locally: half the CPU on DefaultDispatcher, now all on the reactors, +11%)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7c558b59a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
…broke engines that override it, and a breaking change waits for a major release
…are copy on write (ReadMostlyAttributes), a write taking one of 16 locks by identity and computeIfAbsent's block outside it; other concurrent attributes, such as a client request's that every send writes, are a ConcurrentMap again, with no lock every instance shares
… core improvements (core-improvements, ktorio#5983), as the HttpArena entries build them
Bruce Hamilton (bjhham)
left a comment
Bruce Hamilton (bjhham)
left a comment
There was a problem hiding this comment.
Thanks!
The allocation savings look good:
Request consumes 5.68 KB, expected 7.22 KB.
Difference: -1580 <= 111 (allowed)
Saved 1580 bytes on request
(See stdout + build/allocations/* files for details)
Increased locations:
NettyApplicationCall.kt +229 (88 --> 317)
NettyHttp1Handler.kt +31 (720 --> 751)
RoutingPipelineCall.kt +16 (72 --> 88)
DefaultEnginePipeline.kt +8 (88 --> 96)
Decreased locations:
NettyHttpResponsePipeline.kt -552 (849 --> 297)
Pipeline.kt -256 (376 --> 120)
BaseApplicationRequest.kt -168 (200 --> 32)
AttributesJvm.kt -160 (368 --> 208)
RoutingNode.kt -152 (248 --> 96)
Collections.kt -96 (144 --> 48)
BaseApplicationResponse.kt -80 (256 --> 176)
ByteReadChannelOperations.kt -80 (80 --> 0)
NettyApplicationRequestHeaders.kt -72 (120 --> 48)
Parameters.kt -64 (152 --> 88)
StringValues.kt -64 (128 --> 64)
ApplicationSendPipeline.kt -48 (48 --> 0)
RoutingRoot.kt -40 (368 --> 328)
ApplicationReceiveFunctions.kt -32 (32 --> 0)
Just had a few comments on the implementation and one broken engine 😅
| return overridden[name] | ||
| } | ||
|
|
||
| return original[name] |
There was a problem hiding this comment.
Looks like the tests are failing because of some difference in Jetty. I'm guessing things need to happen in a certain order for the headers to populate for this engine...
java.lang.NullPointerException
at io.ktor.server.jetty.jakarta.JettyHeaders.get(JettyHeaders.kt:28)
at io.ktor.server.engine.DelegateHeaders.get(BaseApplicationRequest.kt:89)
at io.ktor.server.engine.BaseApplicationResponse.commitHeaders(BaseApplicationResponse.kt:120)
|
|
||
| // The pipelines a call's receive and send run: its own copies, if something asked for them to intercept this call | ||
| // alone, or else the application's. | ||
| internal fun PipelineRequest.pipelineToRun(): ApplicationReceivePipeline = when (this) { | ||
| is BaseApplicationRequest -> pipelineToRun | ||
| is RoutingPipelineRequest -> pipelineToRun | ||
| else -> pipeline | ||
| } | ||
|
|
||
| internal fun PipelineResponse.pipelineToRun(): ApplicationSendPipeline = when (this) { | ||
| is BaseApplicationResponse -> pipelineToRun | ||
| is RoutingPipelineResponse -> pipelineToRun | ||
| else -> pipeline | ||
| } |
There was a problem hiding this comment.
I think we can introduce the execute() function directly on PipelineRequest and PipelineResponse interfaces and deprecate the pipeline property instead of using this cast here. Then we can delegate to callPipeline ?: call.application.receivePipeline in the request types and avoid this internal val pipelineToRun business.
| override val parameters: Parameters by lazy(LazyThreadSafetyMode.NONE) { | ||
| Parameters.build { | ||
| private var allParameters: Parameters? = null | ||
|
|
||
| override val parameters: Parameters | ||
| get() = allParameters ?: Parameters.build { | ||
| appendAll(engineCall.parameters) | ||
| appendMissing(pathParameters) | ||
| } | ||
| } | ||
| }.also { allParameters = it } |
There was a problem hiding this comment.
Maybe we can avoid the direct translation of lazy properties into nullables? I understand it allocates two objects in native, but this seems like a problem for the std lib. It introduces a bit of cognitive load here.
| // A call's own receive and send pipelines are made only when someone asks for them; until then it runs the | ||
| // application's. What one call adds to its own must still reach that call, wherever it sends from, and no other. | ||
| class CallPipelineCopyTest { |
There was a problem hiding this comment.
Maybe just drop this test - it's asserting things about the internals that don't really have any bearing on correctness.
|
|
||
| fun sharedInterceptors(): MutableList<PipelineInterceptor<TSubject, Call>> { | ||
| shared = true | ||
| // Read first, as in Pipeline.sharedInterceptorsList: every call's pipeline copy comes through here |
There was a problem hiding this comment.
Some of these comments can be removed.
…bsent header instead of throwing - nothing called it until DelegateHeaders.get, so every Jetty response failed in commitHeaders, which reads Connection
…ction not named after its return type; shorter comments
…st.execute and PipelineResponse.execute, and pipeline, which only intercepting a single call needs, is deprecated; routing still merges route interceptors into the pipeline the call runs without copying it; shorter comments, and no test of which pipeline object a call holds
|
Updates addressing change requests and CI issues On jetty engine the problem isn't the ordering, JettyHeaders.get throws on any header the request doesnt have changes on execute(), on both interfaces, default runs pipeline. changes on by lazy - reverted, the allocation cost is reasonable dropped CallPipelineCopyTest Locally CI passes on JDK 21 |
Bruce Hamilton (bjhham)
left a comment
Bruce Hamilton (bjhham)
left a comment
There was a problem hiding this comment.
I'm happy with the changes here, could you mark as "Ready for Review"?
I've created an issue in our tracker to link on the PR title/description for tracking:
KTOR-10131 Performance / allocations audit of common server classes
Should I fix the PR title? |
Bruce Hamilton (bjhham)
changed the title
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5ba96b87d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| } | ||
|
|
||
| override fun get(name: String): String? = jettyRequest.headers.get(name).takeIf { it.isNotEmpty() } | ||
| override fun get(name: String): String? = jettyRequest.headers.get(name)?.takeIf { it.isNotEmpty() } |
There was a problem hiding this comment.
When a Jetty request contains a present-but-empty header such as X-Flag:, HttpFields.get(name) returns "", and this filter converts it to null. The newly added DelegateHeaders.get now calls this override directly, whereas it previously inherited StringValues.get and read getAll, which preserves the empty value, so call.request.headers["X-Flag"] now incorrectly reports the header as absent; return the nullable Jetty value without filtering empty strings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Might want to check this one Diogo Martins (@MDA2AV), though iirc missing headers can come back as empty instead of null so it might be impossible to differentiate now.
There was a problem hiding this comment.
Yes this is a bug, for jetty get and getAll wouldn't agree anymore, I'll take a look into this and the other engines too
There was a problem hiding this comment.
Bruce Hamilton (@bjhham)
Ok, on this issue (sorry for long answer);
The idea of these changes are to start using this get instead of getAll which allocates a list every time. This however resurfaced 2 pre existing bugs on main
override fun get(name: String): String? = jettyRequest.headers.get(name).takeIf { it.isNotEmpty() }
This returns the first value or null if header doesn't exist.
So in case of:
Missing header it would throw (get return nulls, takeIf would run it.isNotEmpty() on that null for a NullPointerException)
Empty header as for example as you mentioned an "X-Flag:", Jetty returns "" and takeIf {it.isNotEmpty()} turns that into null.
main never notices these 2 issues because the method is never called there, call.request.headers is DelegateHeaders which has no get on main so headers[name] uses the default on StringValues.kt:49 getAll(name)?.firstOrNull() which goes to JettyHeaders.getAll. This returns null for a missing header and [""] for an empty one.
So, timeline adds up, once we gave DelegateHeaders its own get on this pr, the bug showed right away as you mentioned initially, jetty engine was broken with the stack trace
java.lang.NullPointerException
at io.ktor.server.jetty.jakarta.JettyHeaders.get(JettyHeaders.kt:28)
at io.ktor.server.engine.DelegateHeaders.get(BaseApplicationRequest.kt:89)
at io.ktor.server.engine.BaseApplicationResponse.commitHeaders(BaseApplicationResponse.kt:120)
So, jetty was breaking here because most request dont send Connection and jetty's get("Connection") returned null.
To align behavior with getAll my proposal here is to have just
override fun get(name: String): String? = jettyRequest.headers.get(name)
so we return "" instead of null which was the result of the takeIf(false).
If you agree, I'll proceed with the change, I am unaware on why there was a call to do things this way on jetty, all other engines are working as expected, get and getall match behavior.
Other investigation vectors worth mentioning
Could the reason why only getAll being used be related to repeated headers? Doesn't seem like because even going though getAll only, it would only return .firstOrNull().
So tracing back to why main goes through getAll to get a single header (taken from an AI agent investigation):
"Why main went through getAll:
- DelegateHeaders was added in 2023 for KTOR-6080, "Decompressed response/request should unset Content-Encoding". It exists so plugins can change or remove request headers, for example dropping Content-Encoding after the body has been decompressed.
- It only implements getAll, names, entries and isEmpty, and leaves get and contains to the StringValues defaults, which are built on getAll. Overriding getAll alone was enough for every read to see the changes.
So it looks like the result of implementing as little as possible, not a choice about repeated headers. Nothing in that code mentions them. Our get applies the same rules: a removed header gives null, a changed one gives its new first value, and anything else goes to the engine."
There was a problem hiding this comment.
I think we can just remove .takeIf { it.isNotEmpty() }. Looks like Jetty returns null when it's unavailable and this filter was misguided / motivated by some spooky past behaviour.
| override fun get(name: String): String? = jettyRequest.headers.get(name)?.takeIf { it.isNotEmpty() } | |
| override fun get(name: String): String? = jettyRequest.headers.get(name) |
Subsystem
Server, Core
Motivation
KTOR-10131 Performance / allocations audit of common server classes
Solution
This PR description is not written by an AI agent.
This PR gathers all the server core and utils changes that were in #5976. All this changes were found when optimizing ktor for a synthetic benchmark platform.
Overall summary:
Most of the changes cut what a call allocates in ktor-utils and ktor-server-core:
Summarized notes for each commit:
e0efe55, ktor-utils: On Native, non-concurrent Attributes use a plain HashMap, and attribute maps are created only on first write. AttributeKey caches its hash. Pipeline uses a @ Volatile field instead of an atomicfu box. The shared-list flags are only written when they actually change.
f370860, ktor-server-core: A call's receive and send pipelines are copied only when something asks for them. Routing reads the query lazily, with % escapes still validated before the handler. There's one RoutingCall per call. Lazy holders and atomicfu boxes are gone, and header overrides are allocated only when used. BaseApplicationRequest.pipeline becomes final. Adds CallPipelineCopyTest.
ff63083, ktor-utils: Concurrent attributes on Native are copy on write, so reads take no lock. This removes the per-call lock that route-scoped plugin lookups took across all threads.
c7c558b, ktor-server-compression: Compression deflates in the call's context instead of on Dispatchers.Default, so each response no longer crosses threads twice.
AI usage must be disclosed. AI must not be used for communication.
See our AI Usage Policy.