Skip to content

[ISSUE #10968] Copy transaction message properties directly instead of encode/decode roundtrip - #10970

Open
wang-jiahua wants to merge 1 commit into
apache:developfrom
wang-jiahua:perf/end-transaction-properties-copy
Open

[ISSUE #10968] Copy transaction message properties directly instead of encode/decode roundtrip#10970
wang-jiahua wants to merge 1 commit into
apache:developfrom
wang-jiahua:perf/end-transaction-properties-copy

Conversation

@wang-jiahua

Copy link
Copy Markdown
Contributor

Which Issue(s) This PR Fixes

Fixes #10968

Brief Description

EndTransactionProcessor#endMessageTransaction deep-copied the prepare message's properties by encoding them to a String and immediately decoding that String back to a map:

MessageAccessor.setProperties(msgInner,
    MessageDecoder.string2messageProperties(MessageDecoder.messageProperties2String(msgExt.getProperties())));

That is a full map traversal + StringBuilder + String, then a second traversal + per-entry substring allocations — on every transaction commit/rollback — just to copy a map. This PR replaces the round-trip with MessageAccessor.deepCopyProperties(...). The real encode after clearProperty (which produces the wire-format propertiesString) is untouched, so the stored message is byte-identical.

Behavior parity note: the old round-trip silently dropped entries with null/empty values, but at this call site msgExt is decoded from the store — the properties parser never yields null/empty values and putUserProperty rejects blank values, so that path is unreachable here.

How Did You Test This Change?

  • EndTransactionProcessorTest passes (7/7).
  • A/B benchmark on a 4-node cluster (8C32G, ESSD): benchmark.TransactionProducer, 32 threads, 1 KiB body, all-commit; per arm clean store + broker restart + page cache drop; 25 s warmup + 60 s collect; 3 interleaved trials:
trial arm TPS young GC GC / million msgs send failed
1 base 64,927 25 6.42 0
1 patch 66,767 24 5.99 0
2 base 66,078 26 6.56 0
2 patch 64,868 24 6.17 0
3 base 66,781 25 6.24 0
3 patch 65,184 24 6.14 0

Broker young GC per million messages: median 6.42 -> 6.14 (-4.4%), patch below base in all three trials; TPS flat; zero send failures.

Copilot AI lite review requested due to automatic review settings August 27, 2026 06:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes transaction end-message handling in the broker by replacing a properties encode/decode round-trip (used solely for deep-copying) with a direct map copy via MessageAccessor.deepCopyProperties(...), reducing unnecessary allocations during transaction commit/rollback.

Changes:

  • Replace MessageDecoder.messageProperties2String(...) + MessageDecoder.string2messageProperties(...) copy round-trip with MessageAccessor.deepCopyProperties(...) in EndTransactionProcessor#endMessageTransaction.
  • Preserve the existing final propertiesString encode after clearProperty, keeping persisted wire-format behavior unchanged.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Replacing the encode/decode roundtrip with MessageAccessor.deepCopyProperties(...) is a clean improvement. The method already exists and is used in TimerMessageStore and TimerMessageRocksDBStore, so this is consistent with established patterns. The behavior parity analysis in the PR description is thorough — the null/empty value path is indeed unreachable at this call site since msgExt properties come from the store decoder.


Automated review by github-manager-bot

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.50%. Comparing base (e348efa) to head (8c664ed).

Additional details and impacted files
@@              Coverage Diff              @@
##             develop   #10970      +/-   ##
=============================================
- Coverage      48.56%   48.50%   -0.07%     
+ Complexity     13672    13652      -20     
=============================================
  Files           1381     1381              
  Lines         101475   101475              
  Branches       13190    13190              
=============================================
- Hits           49286    49216      -70     
- Misses         46185    46243      +58     
- Partials        6004     6016      +12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] Avoid redundant properties encode/decode round-trip in EndTransactionProcessor

4 participants