Repository navigation
Optimize MCP parameter-header processing - #3032
HarshPopat23 wants to merge 5 commits into
Conversation
Add gated MCP benchmarks for parsed metadata, separate JSON parsing, header mirroring, result writers and subscription intersection. Include Release-active fixture controls and reproducible measurement instructions. Signed-off-by: HarshPopat23 <musichk61@gmail.com>
Borrow descriptor names, avoid small-list set allocations and index larger mirrored-header collections. Preserve duplicate and exact numeric behavior. Add indexed-path regression controls and scoped comparative measurements. Signed-off-by: HarshPopat23 <musichk61@gmail.com>
🤖 Augment PR SummaryOptimizes MCP parameter-header processing to reduce repeated allocations and scans.
🤖 Was this summary useful? React with 👍 or 👎 |
There was a problem hiding this comment.
4 issues found across 6 files
You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="benchmark/MCP-header-results.markdown">
<violation number="1" location="benchmark/MCP-header-results.markdown:5">
P3: 'PR 1' is undefined. Refer to the baseline PR by its number (#3031) or to the fixture definitions in `benchmark/MCP.markdown`.</violation>
<violation number="2" location="benchmark/MCP-header-results.markdown:5">
P2: The environment description conflicts with the methodology in `benchmark/MCP.markdown`, which requires an otherwise idle machine. A shared runner is not idle. Add the commit SHA, CPU/OS, system load, SIMD flags and `cmake -LA -N build` output that MCP.markdown requires, or note that these runs do not meet that requirement.</violation>
<violation number="3" location="benchmark/MCP-header-results.markdown:7">
P3: The table gives only medians, so the per-run values behind them are not recorded. Include the three baseline and three candidate timings for each case, or link the retained `before-N.json`/`after-N.json` outputs, as the reproduction instructions require.</violation>
</file>
<file name="src/core/mcp/parameter_headers.cc">
<violation number="1" location="src/core/mcp/parameter_headers.cc:177">
P3: The `substr(10)` offset is a magic number tied to the separate "Mcp-Param-" literal. Define one shared prefix constant and derive the offset from its size so the two cannot diverge.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
|
||
| Dependent on the MCP baselines in #3031. Public signatures and validation policy are unchanged. | ||
|
|
||
| Local Linux shared runner, GCC 13.3, CMake Release (including configured LTO/SIMD), system allocator. Identical PR 1 fixtures; three alternating baseline/candidate process runs. Values are the median across runs of the harness realTime statistic (fastest of three measurement windows), not per-request p95. No compilation ran during the retained comparisons. An earlier executable-relink collision failed to run the candidate and was discarded. |
There was a problem hiding this comment.
P2: The environment description conflicts with the methodology in benchmark/MCP.markdown, which requires an otherwise idle machine. A shared runner is not idle. Add the commit SHA, CPU/OS, system load, SIMD flags and cmake -LA -N build output that MCP.markdown requires, or note that these runs do not meet that requirement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At benchmark/MCP-header-results.markdown, line 5:
<comment>The environment description conflicts with the methodology in `benchmark/MCP.markdown`, which requires an otherwise idle machine. A shared runner is not idle. Add the commit SHA, CPU/OS, system load, SIMD flags and `cmake -LA -N build` output that MCP.markdown requires, or note that these runs do not meet that requirement.</comment>
<file context>
@@ -0,0 +1,22 @@
+
+Dependent on the MCP baselines in #3031. Public signatures and validation policy are unchanged.
+
+Local Linux shared runner, GCC 13.3, CMake Release (including configured LTO/SIMD), system allocator. Identical PR 1 fixtures; three alternating baseline/candidate process runs. Values are the median across runs of the harness realTime statistic (fastest of three measurement windows), not per-request p95. No compilation ran during the retained comparisons. An earlier executable-relink collision failed to run the candidate and was discarded.
+
+| Case | Baseline median ns/op | Candidate median ns/op | Change |
</file context>
|
|
||
| Dependent on the MCP baselines in #3031. Public signatures and validation policy are unchanged. | ||
|
|
||
| Local Linux shared runner, GCC 13.3, CMake Release (including configured LTO/SIMD), system allocator. Identical PR 1 fixtures; three alternating baseline/candidate process runs. Values are the median across runs of the harness realTime statistic (fastest of three measurement windows), not per-request p95. No compilation ran during the retained comparisons. An earlier executable-relink collision failed to run the candidate and was discarded. |
There was a problem hiding this comment.
P3: 'PR 1' is undefined. Refer to the baseline PR by its number (#3031) or to the fixture definitions in benchmark/MCP.markdown.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At benchmark/MCP-header-results.markdown, line 5:
<comment>'PR 1' is undefined. Refer to the baseline PR by its number (#3031) or to the fixture definitions in `benchmark/MCP.markdown`.</comment>
<file context>
@@ -0,0 +1,22 @@
+
+Dependent on the MCP baselines in #3031. Public signatures and validation policy are unchanged.
+
+Local Linux shared runner, GCC 13.3, CMake Release (including configured LTO/SIMD), system allocator. Identical PR 1 fixtures; three alternating baseline/candidate process runs. Values are the median across runs of the harness realTime statistic (fastest of three measurement windows), not per-request p95. No compilation ran during the retained comparisons. An earlier executable-relink collision failed to run the candidate and was discarded.
+
+| Case | Baseline median ns/op | Candidate median ns/op | Change |
</file context>
|
|
||
| Local Linux shared runner, GCC 13.3, CMake Release (including configured LTO/SIMD), system allocator. Identical PR 1 fixtures; three alternating baseline/candidate process runs. Values are the median across runs of the harness realTime statistic (fastest of three measurement windows), not per-request p95. No compilation ran during the retained comparisons. An earlier executable-relink collision failed to run the candidate and was discarded. | ||
|
|
||
| | Case | Baseline median ns/op | Candidate median ns/op | Change | |
There was a problem hiding this comment.
P3: The table gives only medians, so the per-run values behind them are not recorded. Include the three baseline and three candidate timings for each case, or link the retained before-N.json/after-N.json outputs, as the reproduction instructions require.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At benchmark/MCP-header-results.markdown, line 7:
<comment>The table gives only medians, so the per-run values behind them are not recorded. Include the three baseline and three candidate timings for each case, or link the retained `before-N.json`/`after-N.json` outputs, as the reproduction instructions require.</comment>
<file context>
@@ -0,0 +1,22 @@
+
+Local Linux shared runner, GCC 13.3, CMake Release (including configured LTO/SIMD), system allocator. Identical PR 1 fixtures; three alternating baseline/candidate process runs. Values are the median across runs of the harness realTime statistic (fastest of three measurement windows), not per-request p95. No compilation ran during the retained comparisons. An earlier executable-relink collision failed to run the candidate and was discarded.
+
+| Case | Baseline median ns/op | Candidate median ns/op | Change |
+|---|---:|---:|---:|
+| MCP_Validate_Headers_2 | 418.99 | 222.58 | -46.88% |
</file context>
| indexed.reserve(headers.size()); | ||
| for (const auto &entry : headers) { | ||
| if (starts_with_ignore_case(entry.first, "Mcp-Param-")) { | ||
| indexed.emplace_back(entry.first.substr(10), entry.second); |
There was a problem hiding this comment.
P3: The substr(10) offset is a magic number tied to the separate "Mcp-Param-" literal. Define one shared prefix constant and derive the offset from its size so the two cannot diverge.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/core/mcp/parameter_headers.cc, line 177:
<comment>The `substr(10)` offset is a magic number tied to the separate "Mcp-Param-" literal. Define one shared prefix constant and derive the offset from its size so the two cannot diverge.</comment>
<file context>
@@ -141,24 +166,57 @@ auto mcp_validate_parameter_headers(
+ indexed.reserve(headers.size());
+ for (const auto &entry : headers) {
+ if (starts_with_ignore_case(entry.first, "Mcp-Param-")) {
+ indexed.emplace_back(entry.first.substr(10), entry.second);
+ }
+ }
</file context>
Use descriptive comparator parameter names for readability-identifier-length. Carry the existing sourcemeta#3031 benchmark lint fixes onto this dependent branch: designated descriptor initialization and explicit arithmetic grouping. No validation or benchmark behavior changes. Signed-off-by: HarshPopat23 <musichk61@gmail.com>
Reserve descriptor capacity and use named aggregate fields. Preserve all test inputs and assertions. Verify all changed C++ files with the full repository clang-tidy configuration and rerun the 1,421-test MCP suite. Signed-off-by: HarshPopat23 <musichk61@gmail.com>
…ymbols For Linux static mimalloc builds, apply WHOLE_ARCHIVE to every reference to the allocator target so empty/LTO executables cannot select libstdc++ new/delete before later extracting the allocator. Keep shared and system-allocator builds unchanged and preserve static runtime linking. Local GCC 13 Release checks: allocator tests pass, empty framework test retains its expected TAP output/exit code, and operator new resolves to mi_new. The reported collision occurs with CI's GCC 15 and requires CI confirmation. Signed-off-by: HarshPopat23 <musichk61@gmail.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmake/common/targets/executable.cmake">
<violation number="1" location="cmake/common/targets/executable.cmake:42">
P2: The comment says this override covers every reference to the allocator, including allocator test targets, but it is set only on targets created through `sourcemeta_executable()`. The stacktrace test programs in `test/stacktrace/CMakeLists.txt` link `${SOURCEMETA_CORE_ALLOCATOR_TARGET}` directly and get no `WHOLE_ARCHIVE` override. Either apply the same property to those targets or narrow the comment to what this function covers.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
View guided diff | Re-trigger cubic
| if(SOURCEMETA_OS_LINUX AND NOT BUILD_SHARED_LIBS AND Mimalloc_FOUND) | ||
| # Load the replacement entry points before the static C++ runtime can | ||
| # supply its own new/delete definitions. Override all references to this | ||
| # target, including explicit allocator links on allocator test targets. |
There was a problem hiding this comment.
P2: The comment says this override covers every reference to the allocator, including allocator test targets, but it is set only on targets created through sourcemeta_executable(). The stacktrace test programs in test/stacktrace/CMakeLists.txt link ${SOURCEMETA_CORE_ALLOCATOR_TARGET} directly and get no WHOLE_ARCHIVE override. Either apply the same property to those targets or narrow the comment to what this function covers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmake/common/targets/executable.cmake, line 42:
<comment>The comment says this override covers every reference to the allocator, including allocator test targets, but it is set only on targets created through `sourcemeta_executable()`. The stacktrace test programs in `test/stacktrace/CMakeLists.txt` link `${SOURCEMETA_CORE_ALLOCATOR_TARGET}` directly and get no `WHOLE_ARCHIVE` override. Either apply the same property to those targets or narrow the comment to what this function covers.</comment>
<file context>
@@ -36,6 +36,13 @@ function(sourcemeta_executable)
+ if(SOURCEMETA_OS_LINUX AND NOT BUILD_SHARED_LIBS AND Mimalloc_FOUND)
+ # Load the replacement entry points before the static C++ runtime can
+ # supply its own new/delete definitions. Override all references to this
+ # target, including explicit allocator links on allocator test targets.
+ set_property(TARGET "${TARGET_NAME}" PROPERTY
+ "LINK_LIBRARY_OVERRIDE_${SOURCEMETA_CORE_ALLOCATOR_TARGET}" WHOLE_ARCHIVE)
</file context>
Summary
Parameter-header processing currently rebuilds descriptor-name validation structures and repeatedly scans the supplied headers. This PR reduces that work while retaining the existing public API and validation behavior.
Depends on #3031. This is a draft stacked on the benchmark PR. After #3031 merges, this branch should be rebased onto
mainso the final diff contains only the optimization, regression coverage and comparative measurements.Changes
Descriptor validation
Header lookup
Behavior preserved
Benchmark results
Measured using the unchanged fixtures from #3031, with three alternating baseline/candidate process runs on the same local Linux runner, GCC 13.3, Release configuration and system allocator.
Values below are medians across process runs of the harness’s per-operation
realTimestatistic. They are not request-latency percentiles.All measured small-input cases improved. Large header generation was approximately 2% slower, so this PR does not claim improvement across every workload.
The complete measurement procedure and limitations are documented in
benchmark/MCP-header-results.markdown.Regression coverage
Adds an indexed-path regression case covering:
Verification
git diff --checkpassed.Limitations
The indexing thresholds are internal heuristics, not descriptor-count limits or a proven optimal crossover.
Measurements cover the existing 2/64-descriptor fixtures with short names. Other cardinalities, longer names, platforms and allocators may behave differently. The full repository test matrix and allocation profiling were not run.
These results describe local helper operations, not end-to-end MCP transport or LLM latency.