[IMPROVED] server: optimize dynamic metadata map and string allocations#8025
[IMPROVED] server: optimize dynamic metadata map and string allocations#8025Emin-ACIKGOZ wants to merge 1 commit intonats-io:mainfrom
Conversation
![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: 7ffb0fc255
ℹ️ 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 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 address that feedback".
| info := streamInfoClusterResponse{OfflineReason: usa.reason, StreamInfo: usa.info} | ||
| s.sendDelayedErrResponse(acc, reply, nil, s.jsonResponse(&info), errRespDelay) | ||
| // The struct is already built and safe to access here. Simply update the timestamp. | ||
| usa.info.TimeStamp = time.Now().UTC() |
There was a problem hiding this comment.
handleClusterStreamInfoRequest now writes usa.info.TimeStamp on every request, but this callback can run concurrently for the same subscription (delivery unlocks before invoking sub.icb in server/client.go), so parallel stream-info requests can race on the shared usa.info struct. Before this change the handler only copied usa.info without mutation, so this introduces a new data race in unsupported-stream info responses.
Useful? React with 👍 / 👎.
| info := consumerInfoClusterResponse{OfflineReason: uca.reason, ConsumerInfo: uca.info} | ||
| s.sendDelayedErrResponse(acc, reply, nil, s.jsonResponse(&info), errRespDelay) | ||
| // The struct is already built and safe to access. Just update the timestamp. | ||
| uca.info.TimeStamp = time.Now().UTC() |
There was a problem hiding this comment.
handleClusterConsumerInfoRequest now assigns uca.info.TimeStamp directly on the shared struct, which is unsafe if multiple consumer-info requests are handled at the same time for this subscription. Since internal callbacks are invoked after releasing the client lock, concurrent deliveries can overlap and race on uca.info; this race was not present when the code only built a response from the precomputed value.
Useful? React with 👍 / 👎.
This change reduces heap churn in setDynamicStreamMetadata and setDynamicConsumerMetadata by pre-allocating map capacity and caching constant string representations of JSApiLevel and VERSION. Benchmark results (go test -bench BenchmarkMetadataAllocations): - Allocations: 3 -> 2 (-33%) - Memory: 656B -> 336B (-48%) - Latency: ~262ns -> ~193ns (-26%) Signed-off-by: Emin-ACIKGOZ <eminsalihacikgoz@gmail.com>
Emin-ACIKGOZ
force-pushed
the
perf/metadata-alloc-optimization
branch
from
7ffb0fc to
2bafa43
Compare
| } | ||
|
|
||
| // Safely pre-compute the static info here. | ||
| var info StreamInfo |
There was a problem hiding this comment.
Could the changes in this file be reverted, don't think it should make a difference whether they are extracted like this or not?
| var ( | ||
| jsApiLevelStr = strconv.Itoa(JSApiLevel) | ||
| natsVerStr = VERSION | ||
| ) |
There was a problem hiding this comment.
| var ( | |
| jsApiLevelStr = strconv.Itoa(JSApiLevel) | |
| natsVerStr = VERSION | |
| ) | |
| var JSApiLevelStr = strconv.Itoa(JSApiLevel) |
VERSION is already a constant string, so only the strconv.Itoa(JSApiLevel) would likely be sufficient here?
| newCfg = *cfg | ||
| } | ||
| newCfg.Metadata = make(map[string]string) | ||
| newCfg := new(StreamConfig) |
There was a problem hiding this comment.
Can this be reverted to var newCfg StreamConfig?
With it I'm getting the following:
BenchmarkMetadataAllocations-16 13251950 89.90 ns/op 0 B/op 0 allocs/op
| @@ -0,0 +1,36 @@ | |||
| package server | |||
There was a problem hiding this comment.
Could these tests be removed?
TestMetadataCorrectnessseems to be already covered byTestJetStreamSetDynamicStreamMetadata- Benchmarking of stream creation (which these metadata paths are part of) are performed as part of
BenchmarkJetStreamCreate
2 participants
Stream and Consumer info requests currently trigger dynamic metadata generation using redundant strconv.Itoa calls and unallocated maps. This results in unnecessary heap churn and map evacuations, especially under heavy monitoring or high traffic.
Resolves #8026
This PR optimizes these hot paths by:
JSApiLevelandVERSIONas package-level globals to eliminatestrconvoverhead.setDynamicStreamMetadataandsetDynamicConsumerMetadatabased on known entry counts, preventing dynamic growth allocations.Benchmark results (isolated via
server/metadata_perf_test.go):The changes made in this PR ensure that metadata overhead remains minimal regardless of polling frequency.
Signed-off-by: Emin Salih Acikgoz eminsalihacikgoz@gmail.com