[ISSUE #10464] Avoid unnecessary allocation in Message.getProperty and NFE in getPriority#10468
[ISSUE #10464] Avoid unnecessary allocation in Message.getProperty and NFE in getPriority#10468wang-jiahua wants to merge 1 commit into
Conversation
…rty and NFE in getPriority - getProperty(): return null when properties map is uninitialized instead of creating an empty HashMap as a side-effect of a read-only call. - getPriority(): add null/empty fast-path before NumberUtils.toInt() to skip the NFE throw+catch when PRIORITY is unset (the common case). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
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
propertieson read ingetProperty(...)(returnnullwhenpropertiesis 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.
| String value = this.getProperty(MessageConst.PROPERTY_PRIORITY); | ||
| if (value == null || value.isEmpty()) { | ||
| return -1; | ||
| } | ||
| return NumberUtils.toInt(value, -1); |
There was a problem hiding this comment.
The early return is intentional and is the whole point of this fix. NumberUtils.toInt(null, -1) internally calls Integer.parseInt(null) which throws a NumberFormatException, catches it, and returns the default. This throw+catch happens on every message because PRIORITY is almost never set. JFR shows ~16,000 NFE/min from this path — each one allocates a stack trace. The null/empty guard eliminates these exceptions entirely while preserving the same return value.
oss-sentinel-ai
left a comment
There was a problem hiding this comment.
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
Which Issue(s) This PR Fixes
Fixes #10464
Brief Description
Two per-message allocation/CPU fixes in
Message.java:getProperty(String): whenproperties == null, returnsnullimmediately instead of creating anew HashMap<>()as a side-effect of a read-only call.getPriority(): adds a null/empty fast-path before delegating toNumberUtils.toInt(). Previously,NumberUtils.toInt(null, -1)internally calledInteger.parseInt(null)which throws and catches aNumberFormatExceptionon every invocation (~16,000 NFE/min under benchmark load). The fast-path skips this throw+catch whenPRIORITYis 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 laterputdirectly intoproperties(without going throughputProperty()) 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 violationsExisting unit tests pass. The behavioral change in
getProperty()is safe becausehasProperty()already handlesproperties == nullby returningfalse, and all write paths go throughputProperty()which creates the HashMap on demand.