wang-jiahua commented on code in PR #10444:
URL: https://github.com/apache/rocketmq/pull/10444#discussion_r3392860488
##########
common/src/main/java/org/apache/rocketmq/common/message/MessageConst.java:
##########
@@ -102,6 +106,18 @@ public class MessageConst {
public static final HashSet<String> STRING_HASH_SET = new HashSet<>(64);
+ // Mirrors STRING_HASH_SET. Used by MessageDecoder.string2messageProperties
+ // to swap freshly-allocated key substrings for the canonical interned
String,
+ // dropping ~50B per well-known key per message under load.
+ public static final Map<String, String> STRING_INTERN_MAP = new
HashMap<>(128);
Review Comment:
Thanks, this is a fair concern. Both intern structures are no longer
publicly exposed:
- `STRING_INTERN_MAP` has been removed entirely. The build logic now
constructs `STRING_INTERN_BY_LEN` directly from the existing `STRING_HASH_SET`,
so there is no longer a separate map for callers to mutate.
- `STRING_INTERN_BY_LEN` was changed from `public` to **package-private**
(`static final String[][]`), so only `MessageDecoder` in the same
`org.apache.rocketmq.common.message` package can read it. External code can no
longer reach the intern table.
Please let me know if you'd prefer an even tighter encapsulation (e.g.
moving it into `MessageDecoder` itself).
##########
common/src/main/java/org/apache/rocketmq/common/message/Message.java:
##########
@@ -103,10 +103,12 @@ public String getUserProperty(final String name) {
}
public String getProperty(final String name) {
+ // Read-only access shouldn't allocate. Original side-effect of
creating an
+ // empty map on every get is wasted when the message has no properties
yet
+ // (e.g. freshly constructed produce request before tags/keys are set).
if (null == this.properties) {
- this.properties = new HashMap<>();
+ return null;
Review Comment:
Thanks for the careful review. After discussing offline, the public-API
side-effect change to `Message.getProperty()`/`getPriority()` has been split
out of this PR into a dedicated PR #10468 so that the behavior change can be
reviewed in isolation with its own compatibility analysis. This PR (#10444) no
longer modifies `Message.java`, so this concern is fully addressed here. Could
you take a look at #10468 when you have time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
