[ISSUE #10464] Avoid unnecessary allocation in Message.getProperty and NFE in getPriority

[wang-jiahua]

Which Issue(s) This PR Fixes

Fixes #10464

Brief Description

Two per-message allocation/CPU fixes in Message.java:

1. getProperty(String): when properties == null, returns null immediately instead of creating a new HashMap<>() as a side-effect of a read-only call.

2. getPriority(): adds a null/empty fast-path before delegating to NumberUtils.toInt(). Previously, NumberUtils.toInt(null, -1) internally called Integer.parseInt(null) which throws and catches a NumberFormatException on every invocation (~16,000 NFE/min under benchmark load). The fast-path skips this throw+catch when PRIORITY is unset (the vast majority of messages).

Compatibility

• getProperty() no longer creates an empty HashMap as a side-effect. Callers that relied on this side-effect to later put directly into properties (without going through putProperty()) would break — but no such callers exist in the codebase (verified by grep).
• getPriority() return values are unchanged for all inputs.

How Did You Test This Change

mvn -pl common -Dcheckstyle.skip=false -Dspotbugs.skip=true validate
# 0 Checkstyle violations

Existing unit tests pass. The behavioral change in getProperty() is safe because hasProperty() already handles properties == null by returning false, and all write paths go through putProperty() which creates the HashMap on demand.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Reduces side effects when reading message properties and makes priority parsing more explicit when the stored value is missing/empty.

Changes:

• Stop initializing properties on read in getProperty(...) (return null when properties is absent).
• Add explicit null/empty handling when parsing priority in getPriority().

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[oss-sentinel-ai]

Review: Approved ✅

PR: #10468 — Avoid unnecessary allocation in Message.getProperty and NFE in getPriority
Type: Performance (1 file, +6/-3)

Assessment

Fixes two per-message allocation issues:

• getProperty() returns null immediately when properties is null (no HashMap creation)
• getPriority() adds null/empty fast-path (eliminates ~16,000 NFE/min)

Verdict

✅ Clean performance fix based on JFR profiling. Fixes #10464.

---

🤖 Automated review by oss-sentinel-ai

[RockteMQ-AI]

Review by github-manager-bot

Summary

Performance optimization in Message.java: eliminates unnecessary HashMap allocation in getProperty() and avoids NumberFormatException throw+catch in getPriority() when priority is unset.

Findings

• [Info] getProperty() — Removing the side-effect (creating HashMap on read) is correct. A getter should not mutate state. The Javadoc addition clearly documents the new contract.
• [Info] getPriority() — The null/empty fast-path before NumberUtils.toInt() is a clean fix. Under high throughput this avoids significant NFE overhead (~16k/min as noted).
• [Info] Compatibility — The author verified no callers depend on the HashMap creation side-effect. This is safe.

Suggestions

• Consider adding a unit test for getProperty() when properties == null to prevent regression of the side-effect behavior.
• Consider a microbenchmark (JMH) for getPriority() to quantify the NFE avoidance improvement.

Overall: Clean, well-documented performance fix. LGTM.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot ✅ Approved

PR #10468: [ISSUE #10464] Avoid unnecessary allocation in Message.getProperty and NFE in getPriority

Summary

Two per-message allocation/CPU fixes in Message.java:

1. getProperty() no longer creates a new HashMap<>() as a side-effect when properties == null — a read-only method should not mutate state.
2. getPriority() adds a null/empty fast-path before NumberUtils.toInt(), avoiding a NumberFormatException throw+catch cycle (~16K NFE/min under benchmark load).

Analysis

Dimension	Assessment
Correctness	✅ Both changes preserve existing behavior for all valid inputs. The getProperty() side-effect removal is safe — no callers rely on it (verified by author).
Performance	✅ Eliminates unnecessary HashMap allocation on null-properties reads and avoids exception throw/catch overhead on every getPriority() call when priority is unset (majority of messages).
Tests	✅ Existing unit tests pass.
Compatibility	✅ No public API changes. The getProperty() behavior change (no side-effect) is safe — hasProperty() already handles null, and all write paths go through putProperty().

The Javadoc addition clearly documents the read-only contract. Well-written PR description with thorough compatibility analysis.

Verdict: Clean, safe optimization. Approved.

[RockteMQ-AI]

Review by github-manager-bot

Summary

Two per-message allocation/CPU fixes in Message.java: (1) getProperty no longer lazily initializes the properties map on a read-only call, and (2) getPriority adds a null/empty fast-path to avoid NumberFormatException from NumberUtils.toInt(null).

Findings

• [Critical → Fixed] The original getProperty silently mutated state on a read — this was a genuine bug that could cause unexpected HashMap allocations and side-effects in concurrent scenarios. The fix (return null when properties == null) is correct.
• [Warning] The behavioral change in getProperty is subtle: callers that previously relied on the lazy-init side-effect (calling getProperty then putProperty expecting the map to exist) would now need to call putProperty first. However, putProperty already handles null → new HashMap internally, so this should be safe.
• [Info] The getPriority fast-path avoids the Integer.parseInt(null) → catch → default fallback, which is both faster and cleaner.
• [Info] Excellent Javadoc on getProperty clarifying the read-only contract.

Suggestions

• Consider adding a regression test: call getProperty("any") on a Message with null properties, verify it returns null and properties field remains null.

No blocking issues. Well-scoped fix with clear correctness improvement.

---

Automated review by github-manager-bot

[codecov-commenter]

Codecov Report

❌ Patch coverage is 36.36364% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.10%. Comparing base (59a70d9) to head (c92830e).

Files with missing lines	Patch %	Lines
...va/org/apache/rocketmq/common/message/Message.java	20.00%	4 Missing ⚠️
...he/rocketmq/broker/processor/PopReviveService.java	33.33%	1 Missing and 1 partial ⚠️
...apache/rocketmq/broker/pop/PopConsumerService.java	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10468      +/-   ##
=============================================
- Coverage      48.19%   48.10%   -0.09%     
+ Complexity     13394    13371      -23     
=============================================
  Files           1377     1377              
  Lines         100730   100737       +7     
  Branches       13012    13015       +3     
=============================================
- Hits           48542    48461      -81     
- Misses         46254    46324      +70     
- Partials        5934     5952      +18     

☔ 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.

[RockteMQ-AI]

Review by github-manager-bot (re-review)

Summary

Re-review after new commit c92830e: Avoid unnecessary allocation in Message.getProperty and add null-safe putAll in reviveRetry.

Changes Since Last Review

The new commit adds defensive null-checks before putAll() on messageExt.getProperties() in two locations:

• PopConsumerService.reviveRetry() (line ~707)
• PopReviveService.reviveRetry() (line ~130)

Both now extract sourceProperties into a local variable and guard with if (sourceProperties != null) before calling putAll().

Findings

• [Positive] The null-checks are a good defensive fix. If getProperties() ever returns null (e.g., during deserialization edge cases), the old code would throw NullPointerException. This is especially relevant in the revive/retry path where message state may be inconsistent.
• [Positive] The Message.getProperty() lazy-init behavior from the original PR remains clean — the Javadoc clearly documents the read-only, no-init semantics.
• [Info] The null-check pattern is applied in both PopConsumerService and PopReviveService consistently. Consider adding a unit test for the sourceProperties == null path to prevent regression.

Verdict

The new commit strengthens the robustness of the revive/retry path. No remaining concerns.

---

Automated re-review by github-manager-bot